fix: address all 15 code review issues (critical/important/suggestions)

Critical: fix double-shutdown in capture.cpp via RAII scoping, fix CoreError
vtable comment. Important: add close_capture/session_status MCP tools (54 total),
replace bare catch(...) with typed catches in snapshot.cpp, prefer NewCapture
message path over filesystem scan, add path traversal validation for exports,
pre-lowercase filter in events.cpp. Suggestions: add min/max/minLength/maxLength
JSON Schema validation, return const char* from string converters, validate
client protocolVersion, consistent ExportResult serialization, use int64_t in
assertCount, add MCP shutdown method, remove stale comment. All 103 tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: JiaboLi-GitHub

src/core/assertions.cpp

@@ -158,25 +158,25 @@ ImageCompareResult assertImage(const std::string& expectedPath,
 // --- assert_count ---
 AssertResult assertCount(const Session& session,
                          const std::string& what,
-                         int expected,
+                         int64_t expected,
                          const std::string& op) {
     auto* ctrl = session.controller();
-    int actual = 0;
+    int64_t actual = 0;
     if (what == "events") {
         auto st = session.status();
-        actual = static_cast<int>(st.totalEvents);
+        actual = static_cast<int64_t>(st.totalEvents);
     } else if (what == "draws") {
         auto draws = listDraws(session, "", UINT32_MAX);
-        actual = static_cast<int>(draws.size());
+        actual = static_cast<int64_t>(draws.size());
     } else if (what == "textures") {
         auto textures = ctrl->GetTextures();
-        actual = static_cast<int>(textures.size());
+        actual = static_cast<int64_t>(textures.size());
     } else if (what == "buffers") {
         auto buffers = ctrl->GetBuffers();
-        actual = static_cast<int>(buffers.size());
+        actual = static_cast<int64_t>(buffers.size());
     } else if (what == "passes") {
         auto passes = listPasses(session);
-        actual = static_cast<int>(passes.size());
+        actual = static_cast<int64_t>(passes.size());
     } else {
         throw CoreError(CoreError::Code::InternalError,
                         "Unknown count target: " + what);

src/core/assertions.h

@@ -28,7 +28,7 @@ ImageCompareResult assertImage(const std::string& expectedPath,
 
 AssertResult assertCount(const Session& session,
                          const std::string& what,
-                         int expected,
+                         int64_t expected,
                          const std::string& op = "eq");
 
 struct CleanAssertResult {

src/core/capture.cpp

@@ -248,75 +248,84 @@ CaptureResult captureFrame(Session& session, const CaptureRequest& req) {
         throw CoreError(CoreError::Code::InternalError,
                         "Failed to connect to target process");
 
-    // RAII guard for ctrl->Shutdown()
+    // RAII guard for ctrl->Shutdown() — single cleanup path, no manual Shutdown()
     struct CtrlGuard {
         ITargetControl* c;
         ~CtrlGuard() { if (c) c->Shutdown(); }
-    } guard{ctrl};
-
-    uint32_t pid = ctrl->GetPID();
+    };
 
-    // 7. Wait for delayFrames, then trigger capture.
+    std::string foundCapture;
+    uint32_t pid = 0;
     {
-        uint32_t waitMs = req.delayFrames * 16; // ~16ms per frame at 60fps
-        if (waitMs < 2000) waitMs = 2000;       // minimum 2s for API init
-        auto waitUntil = std::chrono::steady_clock::now() +
-                         std::chrono::milliseconds(waitMs);
-        while (std::chrono::steady_clock::now() < waitUntil) {
-            if (!ctrl->Connected())
-                throw CoreError(CoreError::Code::InternalError,
-                                "Target process exited during wait");
-            TargetControlMessage msg = ctrl->ReceiveMessage(nullptr);
-            if (msg.type == TargetControlMessageType::Disconnected)
-                throw CoreError(CoreError::Code::InternalError,
-                                "Target process disconnected during wait");
-            std::this_thread::sleep_for(std::chrono::milliseconds(50));
+        CtrlGuard guard{ctrl};
+
+        pid = ctrl->GetPID();
+
+        // 7. Wait for delayFrames, then trigger capture.
+        {
+            uint32_t waitMs = req.delayFrames * 16; // ~16ms per frame at 60fps
+            if (waitMs < 2000) waitMs = 2000;       // minimum 2s for API init
+            auto waitUntil = std::chrono::steady_clock::now() +
+                             std::chrono::milliseconds(waitMs);
+            while (std::chrono::steady_clock::now() < waitUntil) {
+                if (!ctrl->Connected())
+                    throw CoreError(CoreError::Code::InternalError,
+                                    "Target process exited during wait");
+                TargetControlMessage msg = ctrl->ReceiveMessage(nullptr);
+                if (msg.type == TargetControlMessageType::Disconnected)
+                    throw CoreError(CoreError::Code::InternalError,
+                                    "Target process disconnected during wait");
+                std::this_thread::sleep_for(std::chrono::milliseconds(50));
+            }
         }
-    }
 
-    ctrl->TriggerCapture(1);
+        ctrl->TriggerCapture(1);
 
-    // 8. Poll for NewCapture message
-    bool captured = false;
-    auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(60);
+        // 8. Poll for NewCapture message
+        bool captured = false;
+        std::string captureMsgPath; // path from NewCapture message (preferred)
+        auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(60);
 
-    while (std::chrono::steady_clock::now() < deadline) {
-        if (!ctrl->Connected())
-            break;
+        while (std::chrono::steady_clock::now() < deadline) {
+            if (!ctrl->Connected())
+                break;
 
-        TargetControlMessage msg = ctrl->ReceiveMessage(nullptr);
+            TargetControlMessage msg = ctrl->ReceiveMessage(nullptr);
 
-        if (msg.type == TargetControlMessageType::NewCapture) {
-            captured = true;
-            break;
-        }
-        if (msg.type == TargetControlMessageType::Disconnected)
-            break;
+            if (msg.type == TargetControlMessageType::NewCapture) {
+                captured = true;
+                captureMsgPath = std::string(msg.newCapture.path.c_str());
+                break;
+            }
+            if (msg.type == TargetControlMessageType::Disconnected)
+                break;
 
-        std::this_thread::sleep_for(std::chrono::milliseconds(100));
-    }
+            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+        }
 
-    // 9. Find the capture file on disk.
-    // RenderDoc saves captures to its default temp directory (%TEMP%/RenderDoc/)
-    // or the template path we provided.
-    auto exeName = fs::path(req.exePath).stem().string();
-    std::vector<fs::path> searchDirs = {
-        fs::path(captureTemplate).parent_path(),
-        fs::temp_directory_path() / "RenderDoc",
-    };
-    std::string foundCapture = findNewestCapture(exeName, searchDirs);
+        // 9. Find the capture file on disk.
+        // Prefer the path from the NewCapture message (avoids filesystem race).
+        // Fall back to scanning directories if the message path is unavailable.
+        if (!captureMsgPath.empty() && fs::exists(captureMsgPath)) {
+            foundCapture = captureMsgPath;
+        } else {
+            auto exeName = fs::path(req.exePath).stem().string();
+            std::vector<fs::path> searchDirs = {
+                fs::path(captureTemplate).parent_path(),
+                fs::temp_directory_path() / "RenderDoc",
+            };
+            foundCapture = findNewestCapture(exeName, searchDirs);
+        }
 
-    if (foundCapture.empty())
-        throw CoreError(CoreError::Code::InternalError,
-                        captured ? "Capture completed but file not found on disk"
-                                 : "Capture timed out and no capture file found");
+        if (foundCapture.empty())
+            throw CoreError(CoreError::Code::InternalError,
+                            captured ? "Capture completed but file not found on disk"
+                                     : "Capture timed out and no capture file found");
 
-    if (foundCapture != outputPath)
-        fs::copy_file(foundCapture, outputPath, fs::copy_options::overwrite_existing);
+        if (foundCapture != outputPath)
+            fs::copy_file(foundCapture, outputPath, fs::copy_options::overwrite_existing);
 
-    // 10. Cleanup
-    guard.c = nullptr;
-    ctrl->Shutdown();
+    } // 10. guard destructor calls ctrl->Shutdown()
 
     // 11. Auto-open the capture
     session.open(outputPath);

src/core/errors.cpp

@@ -1,8 +1,7 @@
 #include "core/errors.h"
 
-// CoreError is fully inline in the header.
-// This translation unit exists so the CMake target always has at least one .cpp
-// and to anchor the vtable in a single TU.
-namespace renderdoc::core {
-// intentionally empty - vtable anchored by out-of-line destructor if needed
-} // namespace renderdoc::core
+// CoreError destructor is inline (= default in class body).
+// On MSVC, vtable emission with inline destructors works correctly.
+// This TU exists to ensure the CMake target always has at least one .cpp file
+// and to provide a future anchor point if cross-platform needs arise.
+namespace renderdoc::core {} // namespace renderdoc::core

src/core/errors.h

@@ -37,6 +37,8 @@ class CoreError : public std::runtime_error {
     CoreError(Code code, const std::string& message)
         : std::runtime_error(message), m_code(code) {}
 
+    ~CoreError() override = default;
+
     Code code() const { return m_code; }
 
 private:

src/core/events.cpp

@@ -36,10 +36,8 @@ void collectEvents(const rdcarray<ActionDescription>& actions,
             out.push_back(std::move(info));
         } else {
             std::string lowerName = info.name;
-            std::string lowerFilter = filter;
             std::transform(lowerName.begin(), lowerName.end(), lowerName.begin(), ::tolower);
-            std::transform(lowerFilter.begin(), lowerFilter.end(), lowerFilter.begin(), ::tolower);
-            if (lowerName.find(lowerFilter) != std::string::npos)
+            if (lowerName.find(filter) != std::string::npos)
                 out.push_back(std::move(info));
         }
 
@@ -63,10 +61,8 @@ void collectDrawCalls(const rdcarray<ActionDescription>& actions,
             bool matches = true;
             if (!filter.empty()) {
                 std::string lowerName = info.name;
-                std::string lowerFilter = filter;
                 std::transform(lowerName.begin(), lowerName.end(), lowerName.begin(), ::tolower);
-                std::transform(lowerFilter.begin(), lowerFilter.end(), lowerFilter.begin(), ::tolower);
-                matches = (lowerName.find(lowerFilter) != std::string::npos);
+                matches = (lowerName.find(filter) != std::string::npos);
             }
 
             if (matches)
@@ -99,8 +95,12 @@ std::vector<EventInfo> listEvents(const Session& session, const std::string& fil
     auto* ctrl = session.controller(); // throws NoCaptureOpen if not open
     const auto& actions = ctrl->GetRootActions();
 
+    // Pre-lowercase the filter once, rather than per-event.
+    std::string lowerFilter = filter;
+    std::transform(lowerFilter.begin(), lowerFilter.end(), lowerFilter.begin(), ::tolower);
+
     std::vector<EventInfo> result;
-    collectEvents(actions, filter, result);
+    collectEvents(actions, lowerFilter, result);
     return result;
 }
 
@@ -110,8 +110,12 @@ std::vector<EventInfo> listDraws(const Session& session,
     auto* ctrl = session.controller(); // throws NoCaptureOpen if not open
     const auto& actions = ctrl->GetRootActions();
 
+    // Pre-lowercase the filter once, rather than per-draw.
+    std::string lowerFilter = filter;
+    std::transform(lowerFilter.begin(), lowerFilter.end(), lowerFilter.begin(), ::tolower);
+
     std::vector<EventInfo> result;
-    collectDrawCalls(actions, filter, limit, result);
+    collectDrawCalls(actions, lowerFilter, limit, result);
     return result;
 }
 

src/core/export.cpp

@@ -17,6 +17,19 @@ namespace renderdoc::core {
 
 namespace {
 
+// Validate that the resolved output path stays within the intended output directory.
+// Prevents path traversal attacks (e.g., "../../etc/sensitive").
+void validateOutputDir(const std::string& outputDir) {
+    auto canonical = fs::weakly_canonical(fs::path(outputDir));
+    auto canonicalStr = canonical.string();
+    // Reject if the path contains ".." components after canonicalization
+    // (weakly_canonical resolves them, so just check the input).
+    auto input = fs::path(outputDir).lexically_normal().string();
+    if (input.find("..") != std::string::npos)
+        throw CoreError(CoreError::Code::InvalidPath,
+                        "Output directory must not contain path traversal (..): " + outputDir);
+}
+
 // Sanitize a string for use in a filename (replace :: with __ and : with _).
 std::string sanitizeForFilename(const std::string& s) {
     std::string out = s;
@@ -39,6 +52,7 @@ ExportResult exportRenderTarget(const Session& session,
                                 int rtIndex,
                                 const std::string& outputDir)
 {
+    validateOutputDir(outputDir);
     auto* ctrl = session.controller();
 
     // Walk the action tree to find the current event and retrieve its output RT.
@@ -119,6 +133,7 @@ ExportResult exportTexture(const Session& session,
                            uint32_t mip,
                            uint32_t layer)
 {
+    validateOutputDir(outputDir);
     auto* ctrl = session.controller();
 
     ::ResourceId rdcId = fromResourceId(id);
@@ -157,6 +172,7 @@ ExportResult exportBuffer(const Session& session,
                           uint64_t offset,
                           uint64_t size)
 {
+    validateOutputDir(outputDir);
     auto* ctrl = session.controller();
 
     ::ResourceId rdcId = fromResourceId(id);

src/core/snapshot.cpp

@@ -39,6 +39,12 @@ std::string stageToFileSuffix(ShaderStage stage) {
 SnapshotResult exportSnapshot(Session& session, uint32_t eventId,
                               const std::string& outputDir,
                               std::function<std::string(const PipelineState&)> pipelineSerializer) {
+    // Validate output directory for path traversal
+    auto normalizedDir = fs::path(outputDir).lexically_normal().string();
+    if (normalizedDir.find("..") != std::string::npos)
+        throw CoreError(CoreError::Code::InvalidPath,
+                        "Output directory must not contain path traversal (..): " + outputDir);
+
     auto* ctrl = session.controller();
 
     ctrl->SetFrameEvent(eventId, true);
@@ -83,8 +89,12 @@ SnapshotResult exportSnapshot(Session& session, uint32_t eventId,
                     result.files.push_back(path);
                 }
             }
-        } catch (...) {
-            // No shader bound at this stage — skip silently.
+        } catch (const CoreError& e) {
+            // No shader bound at this stage — skip silently for expected errors.
+            if (e.code() != CoreError::Code::NoShaderBound)
+                result.errors.push_back(std::string("Shader export (") + stageToFileSuffix(stage) + "): " + e.what());
+        } catch (const std::exception& e) {
+            result.errors.push_back(std::string("Shader export (") + stageToFileSuffix(stage) + "): " + e.what());
         }
     }
 
@@ -96,17 +106,20 @@ SnapshotResult exportSnapshot(Session& session, uint32_t eventId,
                 // Rename to color{i}.png for snapshot convention.
                 std::string targetName = "color" + std::to_string(i) + ".png";
                 std::string targetPath = (fs::path(outputDir) / targetName).string();
-                // The export already wrote with its own naming; rename it.
                 try {
                     fs::rename(exp.outputPath, targetPath);
                     result.files.push_back(targetPath);
-                } catch (...) {
+                } catch (const std::exception&) {
                     // If rename fails, keep original path.
                     result.files.push_back(exp.outputPath);
                 }
             }
-        } catch (...) {
-            // No RT at this index — skip.
+        } catch (const CoreError& e) {
+            // No RT at this index — skip silently for expected errors.
+            if (e.code() != CoreError::Code::ExportFailed && e.code() != CoreError::Code::InvalidEventId)
+                result.errors.push_back(std::string("RT export (") + std::to_string(i) + "): " + e.what());
+        } catch (const std::exception& e) {
+            result.errors.push_back(std::string("RT export (") + std::to_string(i) + "): " + e.what());
         }
     }
 
@@ -147,8 +160,10 @@ SnapshotResult exportSnapshot(Session& session, uint32_t eventId,
                         result.files.push_back(exp.outputPath);
                     }
                 }
-            } catch (...) {
-                // Depth export failed — not critical.
+            } catch (const CoreError&) {
+                // Depth export failed — not critical for snapshot.
+            } catch (const std::exception& e) {
+                result.errors.push_back(std::string("Depth export: ") + e.what());
             }
         }
     }

src/mcp/mcp_server.cpp

@@ -87,6 +87,11 @@ json McpServer::handleMessage(const json& msg)
         m_initialized = true;
         return nullptr;  // No response for notifications
     }
+    else if(method == "shutdown")
+    {
+        shutdown();
+        return makeResponse(id, json::object());
+    }
     else if(method == "tools/list" || method == "tools/call")
     {
         if(!m_initialized && !isNotification)
@@ -136,8 +141,20 @@ json McpServer::handleInitialize(const json& msg)
 {
     json id = msg.value("id", json(nullptr));
 
+    // Validate client protocol version for compatibility
+    static constexpr const char* kSupportedProtocolVersion = "2025-03-26";
+    json params = msg.value("params", json::object());
+    if (params.contains("protocolVersion")) {
+        std::string clientVersion = params["protocolVersion"].get<std::string>();
+        if (clientVersion != kSupportedProtocolVersion) {
+            return makeError(id, -32602,
+                "Unsupported protocol version: " + clientVersion +
+                " (server supports " + kSupportedProtocolVersion + ")");
+        }
+    }
+
     json result;
-    result["protocolVersion"] = "2025-03-26";
+    result["protocolVersion"] = kSupportedProtocolVersion;
     result["capabilities"]["tools"] = json::object();
     result["serverInfo"]["name"] = "renderdoc-mcp";
     result["serverInfo"]["version"] = "1.0.0";