feat(remote-control): add MCP server core feature#27
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds cpp-mcp as a vendored submodule and CMake integration, declares and registers a new RemoteControl feature, and implements an in-process MCP JSON-RPC server with ImGui settings, session tracking, client config generation, and multiple MCP tools. ChangesRemote Control MCP Server Feature
Sequence DiagramsequenceDiagram
participant ImGui as ImGui (User)
participant RemoteControl
participant Server as mcp::server
participant Client as MCP Client
participant Runtime as Runtime State
ImGui->>RemoteControl: Toggle enable / change settings
RemoteControl->>Server: StartServer() / configure (host,port,tools)
RemoteControl->>Server: RegisterTools()
Server->>Server: run non-blocking
Client->>Server: HTTP MCP request (tool call)
Server->>RemoteControl: invoke tool handler (e.g., inspect)
RemoteControl->>Runtime: query frame_count, VR status, features
Runtime-->>RemoteControl: return state
RemoteControl-->>Server: tool response JSON
Server-->>Client: MCP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/cpp-mcp.cmake (1)
49-53: ⚡ Quick winAdd validation before include pattern replacement to prevent silent configuration failures.
The
string(REPLACE)at lines 49-53 silently no-ops if the upstream include pattern changes (e.g., whitespace variations), which could silently reintroduce the ABI mismatch issue later. Since the submodule has no pinned commit in.gitmodules, a defensive pre-check is warranted.💡 Suggested validation
foreach(_hdr IN LISTS _cpp_mcp_headers) get_filename_component(_name "${_hdr}" NAME) file(READ "${_hdr}" _content) if(_name STREQUAL "mcp_message.h") + if(NOT _content MATCHES "`#include`[ \t]+\"json\\.hpp\"") + message(FATAL_ERROR + "cpp-mcp include pattern mismatch: mcp_message.h does not contain expected '`#include` \"json.hpp\"'. " + "Update cmake/cpp-mcp.cmake patch logic.") + endif() - string(REPLACE - "`#include` \"json.hpp\"" - "`#include` <nlohmann/json.hpp>" - _content "${_content}") + string(REGEX REPLACE + "`#include`[ \t]+\"json\\.hpp\"" + "`#include` <nlohmann/json.hpp>" + _content "${_content}") endif() file(WRITE "${CPP_MCP_PATCHED_INC}/${_name}" "${_content}") endforeach()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/cpp-mcp.cmake` around lines 49 - 53, Before calling string(REPLACE) on _content to swap "`#include` \"json.hpp\"" with "`#include` <nlohmann/json.hpp>", add a validation step that searches _content for the exact include pattern (e.g., match the literal "`#include` \"json.hpp\"" or a narrowly defined regex accounting for common whitespace) and error-out with a clear cmake_fatal_error (or message(FATAL_ERROR)) if the pattern is not found; this ensures the subsequent string(REPLACE) in cmake/cpp-mcp.cmake actually modified something and prevents silent no-ops if the upstream include format changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/RemoteControl.cpp`:
- Around line 50-55: RemoteControl::LoadSettings currently accepts arbitrary
settings.bindAddress and settings.port from persisted JSON; enforce
loopback-only binding and clamp ports here and again in the server start path
(e.g., RemoteControl::Start or whatever launches the listener). Specifically,
when loading, validate settings.bindAddress is a loopback address (accept
"127.0.0.1" and "::1" or resolve/normalize and check isLoopback) and if not,
replace it with the loopback address; also validate/clamp settings.port into the
allowed range (e.g., 1–65535 or your app's allowed port range) and fall back to
the default (8910) on invalid values. Repeat the same checks right before
binding in the start/bind routine to avoid any runtime override from malicious
or stale config.
---
Nitpick comments:
In `@cmake/cpp-mcp.cmake`:
- Around line 49-53: Before calling string(REPLACE) on _content to swap
"`#include` \"json.hpp\"" with "`#include` <nlohmann/json.hpp>", add a validation
step that searches _content for the exact include pattern (e.g., match the
literal "`#include` \"json.hpp\"" or a narrowly defined regex accounting for
common whitespace) and error-out with a clear cmake_fatal_error (or
message(FATAL_ERROR)) if the pattern is not found; this ensures the subsequent
string(REPLACE) in cmake/cpp-mcp.cmake actually modified something and prevents
silent no-ops if the upstream include format changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 628edac9-aafb-4f5e-af00-d43cca0c2d8d
📒 Files selected for processing (11)
.gitmodulesCMakeLists.txtcmake/cpp-mcp.cmakeextern/cpp-mcpfeatures/Remote Control/COREfeatures/Remote Control/Shaders/Features/RemoteControl.inisrc/Feature.cppsrc/Features/RemoteControl.cppsrc/Features/RemoteControl.hsrc/Globals.cppsrc/Globals.h
|
✅ A pre-release build is available for this PR: |
Addresses CodeRabbit Major finding on PR #27: persisted bindAddress and port were applied verbatim, so a config edit could expose the control endpoint off-host (the surface advertises loopback-only). - Centralize validation in IsLoopbackAddress / NormalizeBindAddress / ClampPort helpers in an anonymous namespace. - Apply in LoadSettings so stale/malicious config is corrected at load with a warning. - Re-apply in StartServer immediately before the bind so UI edits or hot-reload can't slip a non-loopback host through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new CORE “Remote Control” feature that embeds an in-process MCP (Model Context Protocol) HTTP server inside CommunityShaders.dll, intended to let external AI/dev tools inspect and manipulate runtime state for investigation and A/B testing.
Changes:
- Adds a vendored
cpp-mcpsubmodule and CMake integration to build/link it as a static library (including an ABI-alignment patch fornlohmann_json). - Implements the
RemoteControlfeature with an ImGui settings panel, server lifecycle, session tracking, and initial MCP tool wiring. - Wires
RemoteControlinto the global feature registry/singletons so it participates in the normal feature lifecycle.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.gitmodules |
Adds extern/cpp-mcp submodule entry. |
CMakeLists.txt |
Includes and links the new cpp-mcp target. |
cmake/cpp-mcp.cmake |
Builds cpp-mcp as a static library and patches headers to align JSON ABI / avoid winsock conflicts. |
features/Remote Control/CORE |
Marks the feature as CORE for packaging/core detection. |
features/Remote Control/Shaders/Features/RemoteControl.ini |
Adds feature version metadata for auto-detected versioning. |
src/Feature.cpp |
Registers RemoteControl in Feature::GetFeatureList(). |
src/Globals.h |
Declares the globals::features::remoteControl singleton. |
src/Globals.cpp |
Defines the RemoteControl singleton instance. |
src/Features/RemoteControl.h |
Declares the new RemoteControl feature class and settings/session state. |
src/Features/RemoteControl.cpp |
Implements server start/stop, UI, session bookkeeping, and MCP tool registration/handlers. |
Comments suppressed due to low confidence (6)
src/Features/RemoteControl.cpp:905
- The console tool description tells callers to poll
get_state, but there is noget_statetool in this PR (the equivalent isinspect(kind='state')). Update the description so clients don’t call a nonexistent tool.
"To verify a state change, poll get_state until "
"frame_count > enqueued_at_frame (at least one tick "
"elapsed), then observe via side channels: tracy "
"captures for perf-affecting changes, "
src/Features/RemoteControl.cpp:800
- The capture tool description references
list_features, but the tool exposed here isfeature(action='list'). Update the text to match the actual tool name/action so clients can follow the instructions.
"be attached or the in-app DLL loaded; check "
"list_features for RenderDoc loaded=true. Output "
"lands in RenderDoc's configured captures dir.\n"
src/Features/RemoteControl.cpp:292
BuildClientConfig()produces an invalid URL whenbindAddressis an IPv6 literal like::1(needs brackets:http://[::1]:PORT/mcp). Either bracket IPv6 literals when formatting or restrictbindAddressto IPv4/hostname values to avoid generating unusable client configs.
{ "type", "http" },
{ "url", std::format("http://{}:{}/mcp",
settings.bindAddress, settings.port) },
} } } }
src/Features/RemoteControl.cpp:417
- The PR description says the initial slice ships only one bootstrap tool (
get_state), but this implementation registers multiple tools and documents feature mutation/capture workflows. Either update the PR description to match what’s shipping, or trim this PR to just the bootstrap tool so scope matches the stated plan.
void RemoteControl::RegisterTools()
{
// Five tools, each semantically rich. Reads vs writes vs lifecycles are
// separated by tool; within each tool, kind/action discriminates the
// specific operation. See agentic-renderdoc's "Why this design" notes —
src/Features/RemoteControl.cpp:567
- These tool handlers run on cpp-mcp’s listener thread, but they read/mutate global engine state and Feature instances (including
Feature::loaded, settings, ABTestingManager, etc.). That introduces C++ data races with the render/game thread and can lead to crashes/undefined behavior. Marshal tool actions onto the main thread (e.g., via SKSE TaskInterface + promise/future) and only touch game/feature state from that thread.
server->register_tool(tool,
[this](const mcp::json& params, const std::string& session_id) -> mcp::json {
RecordToolCall(session_id, "feature");
const std::string action = params.value("action", std::string{});
if (action.empty()) {
src/Features/RemoteControl.cpp:606
feature(action='toggle')directly flipsFeature::loaded. This bypasses the normal boot-time enable/disable flow and can enable features afterState::SetupResources()has already run, leaving required GPU resources uninitialized. Consider restricting toggle to already-loaded features (disable-only), or implement a safe runtime enable/disable path that performs proper setup/teardown on the correct thread.
const bool previous = target->loaded;
target->loaded = desired;
logger::info("Remote Control: feature(toggle, {}, {}) (was {})",
shortName, desired, previous);
86994ef to
150cd0f
Compare
Addresses CodeRabbit Major finding on PR #27: persisted bindAddress and port were applied verbatim, so a config edit could expose the control endpoint off-host (the surface advertises loopback-only). - Centralize validation in IsLoopbackAddress / NormalizeBindAddress / ClampPort helpers in an anonymous namespace. - Apply in LoadSettings so stale/malicious config is corrected at load with a warning. - Re-apply in StartServer immediately before the bind so UI edits or hot-reload can't slip a non-loopback host through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addresses CodeRabbit Major finding on PR #27: persisted bindAddress and port were applied verbatim, so a config edit could expose the control endpoint off-host (the surface advertises loopback-only). - Centralize validation in IsLoopbackAddress / NormalizeBindAddress / ClampPort helpers in an anonymous namespace. - Apply in LoadSettings so stale/malicious config is corrected at load with a warning. - Re-apply in StartServer immediately before the bind so UI edits or hot-reload can't slip a non-loopback host through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
150cd0f to
05922e7
Compare
Copilot findings on PR #27: - DrawSettings panel said the server was bound to 127.0.0.1 "unless explicitly changed", which is no longer true now that NormalizeBindAddress forces non-loopback values back. Reword to "loopback-only — any non-loopback bind address is rejected at load and bind time" so the UI matches the actual policy. - cmake/cpp-mcp.cmake's mcp_message.h patch silently no-op'd if the expected `#include "json.hpp"` ever disappeared upstream, letting the nlohmann_json ABI mismatch silently regress. Verify the substring exists before the REPLACE and emit a FATAL_ERROR pointing at the rationale in the header comment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
src/Features/RemoteControl.cpp:802
- The capture tool description tells clients to check
list_featuresfor RenderDoc status, but nolist_featurestool is registered (feature enumeration is viafeaturewith action='list'). This will mislead MCP clients/agents consuming the tool description; update the text to reference the actual tool/API shape.
"be attached or the in-app DLL loaded; check "
"list_features for RenderDoc loaded=true. Output "
"lands in RenderDoc's configured captures dir.\n"
" screenshot — Lossless screenshot via the "
src/Features/RemoteControl.cpp:606
feature(action='toggle')writes toFeature::loadedfrom the MCP listener thread.loadedis a plain bool that is read frequently on the render thread (e.g., Feature::ForEachLoadedFeature), so this introduces a data race/UB. This should be marshalled onto the main/render thread (e.g., via a command queue drained in State::Reset) or protected with proper synchronization (atomics + well-defined gating).
const bool previous = target->loaded;
target->loaded = desired;
logger::info("Remote Control: feature(toggle, {}, {}) (was {})",
src/Features/RemoteControl.cpp:659
feature(action='set'|'reset')calls Feature::LoadSettings / RestoreDefaultSettings directly on the MCP listener thread. Feature settings are consumed on the main/render thread, and many features touch D3D/game state during these operations, so this is a high risk for races/undefined behavior. Queue these mutations onto the main/render thread and return an acknowledgement token/status instead of mutating immediately on the listener thread.
feature->LoadSettings(blob);
} catch (const std::exception& e) {
return ErrorResult("LoadSettings threw",
{ { "shortName", shortName }, { "detail", e.what() } });
}
logger::info("Remote Control: feature(set, {})", shortName);
return TextResult(mcp::json({
{ "action", "set" },
{ "shortName", shortName },
{ "applied", true },
})
.dump());
}
if (action == "reset") {
try {
feature->RestoreDefaultSettings();
} catch (const std::exception& e) {
Three Copilot findings on PR #27: - State::frameCount is non-atomic and incremented on the render thread, but RemoteControl reads it from the MCP listener thread (inspect kind=state, capture queue, console tick). That's a data race / UB. Add State::frameCountAtomic, a thread-safe mirror written immediately after the existing `frameCount++` on the render thread; route the off-thread reads in RemoteControl through it. The hot path (ScreenSpaceGI, Streamline, Upscaling) keeps using the non-atomic field — no behavior change there. - BuildClientConfig formatted the URL as `http://{host}:{port}/mcp`, which is invalid for the IPv6 loopback "::1" because the URL authority requires bracketed IPv6 literals (RFC 3986 §3.2.2). Detect ':' in bindAddress and wrap it. - The console tool description told agents to "poll get_state", but the tool is `inspect(kind='state')`. Update the description to match what's actually registered. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addresses CodeRabbit Major finding on PR #27: persisted bindAddress and port were applied verbatim, so a config edit could expose the control endpoint off-host (the surface advertises loopback-only). - Centralize validation in IsLoopbackAddress / NormalizeBindAddress / ClampPort helpers in an anonymous namespace. - Apply in LoadSettings so stale/malicious config is corrected at load with a warning. - Re-apply in StartServer immediately before the bind so UI edits or hot-reload can't slip a non-loopback host through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot findings on PR #27: - DrawSettings panel said the server was bound to 127.0.0.1 "unless explicitly changed", which is no longer true now that NormalizeBindAddress forces non-loopback values back. Reword to "loopback-only — any non-loopback bind address is rejected at load and bind time" so the UI matches the actual policy. - cmake/cpp-mcp.cmake's mcp_message.h patch silently no-op'd if the expected `#include "json.hpp"` ever disappeared upstream, letting the nlohmann_json ABI mismatch silently regress. Verify the substring exists before the REPLACE and emit a FATAL_ERROR pointing at the rationale in the header comment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three Copilot findings on PR #27: - State::frameCount is non-atomic and incremented on the render thread, but RemoteControl reads it from the MCP listener thread (inspect kind=state, capture queue, console tick). That's a data race / UB. Add State::frameCountAtomic, a thread-safe mirror written immediately after the existing `frameCount++` on the render thread; route the off-thread reads in RemoteControl through it. The hot path (ScreenSpaceGI, Streamline, Upscaling) keeps using the non-atomic field — no behavior change there. - BuildClientConfig formatted the URL as `http://{host}:{port}/mcp`, which is invalid for the IPv6 loopback "::1" because the URL authority requires bracketed IPv6 literals (RFC 3986 §3.2.2). Detect ':' in bindAddress and wrap it. - The console tool description told agents to "poll get_state", but the tool is `inspect(kind='state')`. Update the description to match what's actually registered. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
0deb63c to
8efcecc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
src/Features/RemoteControl.cpp:612
feature(action='toggle')writesFeature::loadedfrom the MCP listener thread (target->loaded = desired).Feature::loadedis a plainboolthat’s read each frame byFeature::ForEachLoadedFeature, so this introduces a cross-thread data race/UB. Queue toggles onto the main/game thread (e.g., viaSKSE::GetTaskInterface()->AddTaskor a render-thread command queue) and/or make the loaded flag changes synchronized (atomic or mutex-protected).
if (action == "toggle") {
if (!params.contains("enabled") || !params["enabled"].is_boolean()) {
return ErrorResult("missing required boolean parameter 'enabled'");
}
const bool desired = params["enabled"].get<bool>();
// FindFeatureByShortName filters on loaded==true so it can't
// help re-enable; walk the full list ourselves.
Feature* target = nullptr;
for (auto* f : Feature::GetFeatureList()) {
if (f->GetShortName() == shortName) {
target = f;
break;
}
}
if (!target) {
return ErrorResult("feature not found",
{ { "shortName", shortName } });
}
const bool previous = target->loaded;
target->loaded = desired;
logger::info("Remote Control: feature(toggle, {}, {}) (was {})",
shortName, desired, previous);
return TextResult(mcp::json({
src/Features/RemoteControl.cpp:652
feature(action='set')callsFeature::LoadSettingsdirectly from the MCP listener thread. Many features’LoadSettingspaths are assumed to run on the main thread during config load/UI interaction and may touch shared state; invoking them off-thread risks races with the render loop. Consider marshaling settings mutations onto the main/game thread (similar to theconsoletool) and returning an acknowledgment once enqueued/applied.
if (action == "set") {
if (!params.contains("settings") || !params["settings"].is_object()) {
return ErrorResult("missing required object parameter 'settings'");
}
::json blob;
try {
blob = ::json::parse(params["settings"].dump());
} catch (const std::exception& e) {
return ErrorResult("settings is not valid JSON",
{ { "detail", e.what() } });
}
try {
feature->LoadSettings(blob);
} catch (const std::exception& e) {
return ErrorResult("LoadSettings threw",
{ { "shortName", shortName }, { "detail", e.what() } });
}
logger::info("Remote Control: feature(set, {})", shortName);
src/Features/RemoteControl.cpp:668
feature(action='reset')callsFeature::RestoreDefaultSettings()from the MCP listener thread. Restore paths can recreate resources / mutate global state and are typically not thread-safe relative to the render loop. Please enqueue this work onto the main/game thread (or a dedicated render-thread command queue) rather than running it on the listener thread.
if (action == "reset") {
try {
feature->RestoreDefaultSettings();
} catch (const std::exception& e) {
return ErrorResult("RestoreDefaultSettings threw",
{ { "shortName", shortName }, { "detail", e.what() } });
}
logger::info("Remote Control: feature(reset, {})", shortName);
return TextResult(mcp::json({
src/Features/RemoteControl.cpp:763
- The
abtesttool callsABTestingManager::Enable/Disable/SetTestInterval/ClearCachedSnapshotsfrom the MCP listener thread.Enable()/Disable()call intoglobals::state->Load/LoadFromJson/SaveToJsonand flip overlay settings, whileABTestingManager::Update()runs each frame; doing this off-thread risks races and inconsistent state. Queue these operations onto the main/game thread and consider returning the pre/post state once applied.
if (action == "status") {
return TextResult(statusBlob().dump());
}
if (action == "start") {
if (params.contains("interval") && params["interval"].is_number()) {
const auto secs = params["interval"].get<int>();
if (secs > 0) {
mgr->SetTestInterval(static_cast<uint32_t>(secs));
}
}
mgr->Enable();
logger::info("Remote Control: abtest(start)");
return TextResult(statusBlob().dump());
}
if (action == "stop") {
mgr->Disable();
logger::info("Remote Control: abtest(stop)");
return TextResult(statusBlob().dump());
}
if (action == "clear") {
mgr->ClearCachedSnapshots();
logger::info("Remote Control: abtest(clear)");
return TextResult(statusBlob().dump());
Add a new "Remote Control" CORE feature that hosts an embedded Model Context Protocol (MCP) server inside CommunityShaders.dll so AI assistants (Claude Code, Cursor, Continue, etc.) can query and mutate runtime state for A/B testing and performance investigation. Server is off by default and binds to 127.0.0.1 — opt-in via the Settings UI. The ImGui panel exposes a "Copy MCP client config to clipboard" button that emits a Streamable-HTTP (MCP 2025-03-26) config snippet ready to paste into a host's mcpServers settings. Initial slice ships a single bootstrap tool (get_state) so the end-to-end wiring can be validated; subsequent commits will add list_features, get/set/toggle/reset_feature, run_abtest, capture_renderdoc, and capture_screenshot. Library choice: hkr04/cpp-mcp pinned to a0eb22c9 (280 stars, MIT, 2025-03-26 spec compliant including Streamable HTTP). Vendored as a submodule under extern/cpp-mcp — same pattern as FidelityFX-SDK and Streamline — because cpp-mcp has no install rules (upstream PR #12 still open). Only the server-side translation units are compiled; the bundled stdio/SSE *client* implementations are intentionally omitted. The library vendors its own nlohmann_json (3.11.3) which has a different ABI namespace tag than vcpkg's (3.12.0) and would cause LNK2001 on mcp::server::set_capabilities / register_tool if cpp-mcp linked against the vendored copy and consumers against vcpkg. cmake/cpp-mcp.cmake patches mcp_message.h at configure time to use <nlohmann/json.hpp> from vcpkg, writing the patched header to a build-tree mirror so the submodule stays clean. cpp-mcp's vendored cpp-httplib pulls in <winsock2.h>, but Skyrim's PCH transitively brings the legacy <winsock.h> via <Windows.h>. Define _WINSOCKAPI_ on the cpp-mcp target (PUBLIC) so consumers and the PCH skip the legacy header. Wiring: * extern/cpp-mcp submodule pinned to hkr04/cpp-mcp@a0eb22c9 * cmake/cpp-mcp.cmake builds the static library target * features/Remote Control/CORE marker + versioned ini * src/Features/RemoteControl.{h,cpp} (Feature subclass) * registered in globals::features and Feature::GetFeatureList Verified end-to-end: CommunityShaders.dll builds clean, grows ~500 KB, deploys to SE+VR. Off-by-default toggle exposed in the Core Features → Utility group of the in-game menu. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Enumerate all graphics features over MCP. Returns a JSON array with one object per feature including: name, shortName, loaded, version, category, isCore, supportsVR, inMenu. Read-only — iterates Feature::GetFeatureList() without mutation. No render-thread synchronization required. Refactored RegisterTools() into per-tool helpers (RegisterGetStateTool, RegisterListFeaturesTool) so subsequent commits can drop in additional tools without ballooning a single function. Pulled the MCP content envelope into a TextResult() helper shared across tools. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Return the JSON settings blob for a single feature by shortName.
Round-trips through Feature::SaveSettings(), so what comes out is
exactly what the on-disk config encodes — the same shape that
set_feature_settings (next commit) will accept back.
Read-only. No mutation, no render-thread sync required.
Also introduces ErrorResult() — a small helper that emits a
single-text-item content envelope with {"error": "...", ...}.
Conventions for all future tools:
* success → {"...": ...} (tool-specific shape)
* failure → {"error": "<reason>", "<context>": ...}
Callers can always JSON.parse() the text field.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Enable or disable a graphics feature at runtime by flipping Feature::loaded. ForEachLoadedFeature skips features whose loaded flag is false, so every per-frame entrypoint (Prepass, Reflections, Reset, etc.) becomes a no-op for that feature on the next frame. GPU resources allocated during SetupResources() are NOT freed — the goal is fast A/B perf/quality comparisons, not memory reclaim. Re-toggling restores rendering with whatever state the feature had mid-flight. Atomic single-bool write — no command queue required. The listener thread sets `loaded`, the render thread reads it on its next pass. A torn read isn't possible (single byte), and the worst case from a race is "one extra frame of new state" which is the desired outcome anyway. Walks GetFeatureList() directly instead of FindFeatureByShortName() because the latter filters on loaded==true (built for "find an active feature") — we need to be able to re-enable disabled ones. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace a feature's settings with a supplied JSON blob via the existing Feature::LoadSettings(json&) path. Round-trips through dump/parse to bridge cpp-mcp's ordered_json to plain nlohmann::json expected by the feature interface. Handler runs on the cpp-mcp listener thread. Most features only deserialize into member variables here (same pattern the ImGui menu uses), and a few set a recompileFlag that the render loop consumes on the next frame (e.g. ScreenSpaceGI, DynamicCubemaps). Both patterns are safe at this thread boundary. Settings whose LoadSettings synchronously reallocates GPU resources would race the renderer; none in-tree currently do, but documented in the tool description as a follow-up to tighten with a command queue if a future feature lands one. LightLimitFix is a known no-override case: set_feature_settings will return applied=true but the feature's runtime state won't change because LightLimitFix doesn't implement LoadSettings. Tracked separately as a feature-side fix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Restore a feature to its built-in defaults via
Feature::RestoreDefaultSettings(). Semantically distinct from
set_feature_settings({}) — RestoreDefaultSettings is per-feature
reset logic (may release or recreate state that an empty-blob
LoadSettings would not touch).
Useful as the "B" side of A/B comparisons: snapshot with
get_feature_settings, reset, observe, then restore with
set_feature_settings.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Singular MCP tool for the entire console concern. Queues the
command onto the main game thread via SKSE::GetTaskInterface()->
AddTask so RE::Console::ExecuteCommand runs on the next tick,
honoring Skyrim's script-VM main-thread requirement.
Fire-and-forget by design: RE::Console::ExecuteCommand is void
and RE::ConsoleLog is a shared sink with no command-to-output
correlation; many useful commands (tcl, tfc, tg, tm…) are silent.
Scraping the log would produce misleading positives often enough
to be worse than not exposing it. To verify state changes,
callers should poll get_state.frame_count past the returned
enqueued_at_frame and observe via tracy/renderdoc/screenshot
side channels.
Response shape:
{ "queued": true,
"command": "…",
"enqueued_at_frame": N }
Following the agentic-renderdoc philosophy of fewer, semantically
rich tools, this is one tool rather than e.g. execute_console,
read_console_log, get_console_history. Future console-related
capabilities get added as optional parameters on this same tool,
not as new top-level tools.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Single MCP tool for all frame-capture flavors, kind-dispatched.
Per the agentic-renderdoc design philosophy, one rich tool
beats two parallel tools (capture_renderdoc, capture_screenshot)
that would each need their own description and increase agent
decision cost. Future kinds (tracy snapshot, video clip) extend
the same tool.
Supported kinds:
- renderdoc: wraps existing RenderDoc::TriggerCapture /
TriggerMultiFrameCapture. Honors `frames` (1-120,
default 1). Returns 400-equivalent if RenderDoc
feature isn't loaded or in-app DLL isn't attached.
- screenshot: signals ScreenshotFeature::captureRequested
atomic (existing lossless non-blocking path).
`frames` ignored.
Fire-and-forget; no artifact path is returned. RenderDoc captures
land in its configured dir; screenshots in Screenshots/. Callers
must observe via the filesystem (or, for renderdoc, attach the UI).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
In-game visibility for who's currently driving the MCP server. Sortable ImGui table showing session id, connect time, idle time, request count, and last tool invoked. Tracked entirely in-process — no MCP tool exposes this; it's a human-debugging surface. Bookkeeping is updated on every tool handler invocation (lambda captures `this` and calls RecordToolCall(session_id, toolName) before the existing body) and on cpp-mcp's session-cleanup callback when a client disconnects. A std::mutex protects the session map across the listener and main threads. Per-session force-disconnect is intentionally not surfaced — cpp-mcp's public API doesn't expose it cleanly. The settings UI documents that toggling "Enable MCP server" off/on serves as the panic-button "kick everyone". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Single MCP tool driving the full A/B testing lifecycle in
features/Performance Overlay/ABTesting. Action-dispatched per the
agentic-renderdoc design: one rich tool instead of separate
start_abtest / stop_abtest / get_abtest_results / clear_snapshots /
set_interval tools.
Actions:
status — return enabled, usingTestConfig, interval,
hasCachedSnapshots
start — Enable() the rotation. Optional `interval` parameter
(seconds) applied first if provided.
stop — Disable(). Snapshots retained.
clear — ClearCachedSnapshots().
diff — return the per-setting USER vs TEST diff list so
callers know exactly what the rotation is toggling.
Setup of the TEST config itself remains in the Performance Overlay
UI — this tool drives lifecycle only, not test-config authoring.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The capture tool dereferences globals::features::renderDoc and ::screenshotFeature, which were only forward-declared via Globals.h. The previous commit (118e2e5) compiled because nothing yet touched those pointers — adding the tool exposed the missing includes. Pull RenderDoc.h and ScreenshotFeature.h so the member calls (IsAvailable, TriggerCapture, TriggerMultiFrameCapture, captureRequested.store) resolve. Folding this into the capture commit would require an interactive rebase (against project guidance); leaving as a separate fix so each commit stays self-contained and bisect-correct from this point forward. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per the agentic-renderdoc design (https://github.com/alandtse/ agentic-renderdoc#why-this-design), "fewer, semantically rich tools outperform expansive tool suites" — the tool description IS the prompt engineering and each tool's surface should encode operational expertise rather than expose one-method-per-call. We weren't shipped, so collapse the 9-tool surface to 5: inspect(kind) — non-feature engine reads (currently kind='state'; extensible) feature(action, ...) — all feature ops: list / get / set / reset / toggle console(command) — unchanged capture(kind, frames?) — unchanged abtest(action, interval?) — unchanged Removed tools (folded into the new two): get_state → inspect(kind='state') list_features → feature(action='list') get_feature_settings → feature(action='get', shortName) set_feature_settings → feature(action='set', shortName, settings) reset_feature_settings → feature(action='reset', shortName) toggle_feature → feature(action='toggle', shortName, enabled) The two new tool descriptions are deliberately rich (per the philosophy): each documents every action's params, the timing / threading semantics, the A/B testing flow as a worked example, and the known gotchas (LightLimitFix's missing SaveSettings/ LoadSettings override means get returns null and set is a silent no-op; toggle keeps GPU resources alive). Shared helpers (EngineStateBlob, FeatureEntry) are factored as static-file-locals so the same blobs can be reused if we add future tools that surface the same data in different contexts. The ImGui session-tracking table continues to work — handlers still call RecordToolCall(session_id, "<name>") inline; only the recorded names change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addresses CodeRabbit Major finding on PR #27: persisted bindAddress and port were applied verbatim, so a config edit could expose the control endpoint off-host (the surface advertises loopback-only). - Centralize validation in IsLoopbackAddress / NormalizeBindAddress / ClampPort helpers in an anonymous namespace. - Apply in LoadSettings so stale/malicious config is corrected at load with a warning. - Re-apply in StartServer immediately before the bind so UI edits or hot-reload can't slip a non-loopback host through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot findings on PR #27: - DrawSettings panel said the server was bound to 127.0.0.1 "unless explicitly changed", which is no longer true now that NormalizeBindAddress forces non-loopback values back. Reword to "loopback-only — any non-loopback bind address is rejected at load and bind time" so the UI matches the actual policy. - cmake/cpp-mcp.cmake's mcp_message.h patch silently no-op'd if the expected `#include "json.hpp"` ever disappeared upstream, letting the nlohmann_json ABI mismatch silently regress. Verify the substring exists before the REPLACE and emit a FATAL_ERROR pointing at the rationale in the header comment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three Copilot findings on PR #27: - State::frameCount is non-atomic and incremented on the render thread, but RemoteControl reads it from the MCP listener thread (inspect kind=state, capture queue, console tick). That's a data race / UB. Add State::frameCountAtomic, a thread-safe mirror written immediately after the existing `frameCount++` on the render thread; route the off-thread reads in RemoteControl through it. The hot path (ScreenSpaceGI, Streamline, Upscaling) keeps using the non-atomic field — no behavior change there. - BuildClientConfig formatted the URL as `http://{host}:{port}/mcp`, which is invalid for the IPv6 loopback "::1" because the URL authority requires bracketed IPv6 literals (RFC 3986 §3.2.2). Detect ':' in bindAddress and wrap it. - The console tool description told agents to "poll get_state", but the tool is `inspect(kind='state')`. Update the description to match what's actually registered. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
8efcecc to
a8a2d96
Compare
…eader GLOB Address remaining Copilot review comments on PR #27: - RemoteControl.cpp uses std::clamp/std::sort (algorithm) and std::vector (vector) directly. Include the headers explicitly instead of relying on transitive includes from imgui/cpp-mcp, which could break on PCH/toolchain changes. - cmake/cpp-mcp.cmake's file(GLOB) for cpp-mcp headers now uses CONFIGURE_DEPENDS so adding/removing a header in the submodule triggers a CMake reconfigure on the next build. Otherwise the patched header mirror could go stale across submodule updates with no obvious failure.
Address follow-up review comments on PR #27: Thread safety — route Feature/ABTesting mutations onto the main thread via SKSE::GetTaskInterface(), mirroring the existing console tool. Direct writes from the MCP listener thread race with the render loop reading the same state: - feature(action='toggle') now AddTask()s the Feature::loaded write; response includes 'previous', 'requested', and 'enqueued_at_frame'. - feature(action='set') and feature(action='reset') now AddTask() the LoadSettings / RestoreDefaultSettings call. LoadSettings can touch UI/render-thread-visible state inside features; doing it off-thread was a real race. - abtest(action='start'/'stop'/'clear') now AddTask() the Enable/Disable/Clear calls. ABTestingManager::Enable swaps configs via State::Load and Menu::Load, which expect main-thread access. - abtest(action='status') remains synchronous — it only reads. Tool return contract changes: mutations now respond with { queued: true, enqueued_at_frame: N } instead of synchronous applied/reset flags. Callers wanting confirmation should re-query with the corresponding read action. Security — IsLoopbackAddress() no longer treats "localhost" as loopback. On Windows the hosts file or a hijacked resolver can map "localhost" to a non-loopback address, which would silently expose the control endpoint off-host. Only literal 127.0.0.1 / ::1 pass. Docs — capture tool hints now point at feature(action='list') instead of the non-existent list_features tool. The RemoteControl.h forward-declare comment no longer references a "tool-tracking layer" the class never had; it now explains that mcp::json (ordered_json alias) is what blocks moving registration into the header.
Summary
Adds a new Remote Control CORE feature that hosts an embedded Model Context Protocol (MCP) server inside
CommunityShaders.dll, so AI assistants (Claude Code, Cursor, Continue, etc.) can query and mutate runtime state for A/B testing and performance investigation.127.0.0.1— opt-in from the Settings UI.mcpServerssettings.get_state) to validate the wiring end-to-end. Subsequent commits will addlist_features,get/set/toggle/reset_feature,run_abtest,capture_renderdoc,capture_screenshot.Library choice —
hkr04/cpp-mcpPinned to
a0eb22c9. 280 stars, MIT, 2025-03-26 spec compliant including Streamable HTTP. Compared againstgopher-mcp(26× larger, enterprise filter-chain arch) andpeppemas/mcp_server(low activity);cpp-mcpis the right fit for an in-process server inside a game DLL.Vendored as a git submodule under
extern/cpp-mcp— same pattern we use forFidelityFX-SDKandStreamline— because cpp-mcp has no install rules (upstream PR #12 still open). Only server-side translation units are compiled; the bundled stdio/SSE client implementations are intentionally omitted.Two non-obvious integration fixes (captured in
cmake/cpp-mcp.cmake)nlohmann_jsonABI mismatch. cpp-mcp bundles 3.11.3, vcpkg ships 3.12.0. Both wrap their public API in an ABI-versioned inline namespace (json_abi_v3_11_3vsjson_abi_v3_12_0), so even sharing an include guard wasn't enough — cpp-mcp's TUs and our consumer TUs would emit different mangled symbols →LNK2001onset_capabilities/register_tool. Fix: patchmcp_message.hat configure time to use<nlohmann/json.hpp>from vcpkg, writing the patched header to a build-tree mirror so the submodule stays clean.<winsock2.h>; Skyrim's PCH transitively brings legacy<winsock.h>via<Windows.h>. Define_WINSOCKAPI_on the cpp-mcp target (PUBLIC) so consumers and the PCH skip the legacy header.Files
11 files, +412 / -0.
Test plan
BuildRelease.bat ALL-WITH-AUTO-DEPLOYMENTsucceeds with the feature wired inCommunityShaders.dllgrows ~500 KB (cpp-mcp + bundled httplib code), deploys to SE + VR Data folders{"mcpServers": {"community-shaders": {"type": "http", "url": "http://127.0.0.1:8910/mcp"}}})get_statereturns frame counterVRpreset)Follow-ups (separate PRs)
list_features/get_feature/set_feature/toggle_feature/reset_featuretoolsrun_abtestwrappingABTestingManagercapture_renderdoc(frames=N)andcapture_screenshot()wrapping the existing triggers🤖 Generated with Claude Code
Summary by CodeRabbit