Conversation
|
@cyberkaida let me know what improvements you want me to make to this rough version. |
0528da1 to
2109fc2
Compare
|
I did some cleanup on how the errors were being handled and stuff, this doesn't include code cleanup just yet, just fixing some of the issues with how it was handling errors. |
2109fc2 to
73ca9e6
Compare
73ca9e6 to
e40efb1
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds class reconstruction support by implementing a new tool provider for RTTI-based class reconstruction and integrating it into the MCP server.
- Introduces ClassToolProvider with multiple tool registrations for listing, creating, and reconstructing classes via RTTI data.
- Updates integration tests with ClassToolProviderIntegrationTest and registers the new provider in the MCP server.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test.slow/java/reva/tools/classes/ClassToolProviderIntegrationTest.java | Integration tests for the new class reconstruction functionality |
| src/main/java/reva/tools/classes/ClassToolProvider.java | New tool provider implementation with several tools for class operations, including RTTI reconstruction |
| src/main/java/reva/tools/AbstractToolProvider.java | Added a generic helper method to retrieve optional list parameters |
| src/main/java/reva/server/McpServerManager.java | Updated to register the new ClassToolProvider |
| // Execute the RTTI script | ||
| String scriptName = "RecoverClassesFromRTTIScript.java"; | ||
|
|
||
| try { |
There was a problem hiding this comment.
The executeRttiReconstruction method contains deeply nested try-catch blocks. Consider refactoring this logic (e.g., by extracting helper methods) to improve readability and maintainability.
| try { | ||
| // Create the class namespace | ||
| Namespace classNamespace = symbolTable.createClass(parentNamespace, className, ghidra.program.model.symbol.SourceType.USER_DEFINED); | ||
|
|
||
| program.endTransaction(txId, true); | ||
|
|
||
| Map<String, Object> result = createBasicClassInfo(classNamespace, 0); | ||
| result.put("message", "Successfully created class namespace: " + className); | ||
| return createJsonResult(result); | ||
|
|
||
| } catch (Exception e) { | ||
| program.endTransaction(txId, false); | ||
| throw e; |
There was a problem hiding this comment.
Consider using a try-finally block when managing transactions to ensure that program.endTransaction is always executed, even if an exception is thrown.
| try { | |
| // Create the class namespace | |
| Namespace classNamespace = symbolTable.createClass(parentNamespace, className, ghidra.program.model.symbol.SourceType.USER_DEFINED); | |
| program.endTransaction(txId, true); | |
| Map<String, Object> result = createBasicClassInfo(classNamespace, 0); | |
| result.put("message", "Successfully created class namespace: " + className); | |
| return createJsonResult(result); | |
| } catch (Exception e) { | |
| program.endTransaction(txId, false); | |
| throw e; | |
| boolean success = false; | |
| try { | |
| // Create the class namespace | |
| Namespace classNamespace = symbolTable.createClass(parentNamespace, className, ghidra.program.model.symbol.SourceType.USER_DEFINED); | |
| success = true; | |
| Map<String, Object> result = createBasicClassInfo(classNamespace, 0); | |
| result.put("message", "Successfully created class namespace: " + className); | |
| return createJsonResult(result); | |
| } catch (Exception e) { | |
| throw e; | |
| } finally { | |
| program.endTransaction(txId, success); |
e40efb1 to
19b7aaf
Compare
- Use McpSchema.Tool.builder() pattern instead of constructor - Add title fields to all 8 class tools for better UX - Fix address formatting to use AddressUtil.formatAddress() - Update error message to match integration test expectations - Remove unused imports and add missing AddressUtil import This aligns the ClassToolProvider with the new MCP SDK patterns introduced in the main branch rebase.
19b7aaf to
79fb394
Compare
This is a PR for class reconstruction support. It's still a work in progress, but it's working. We need to clean it up probably.