feat: Implement Phase 31 Data and Persistence manage_data tool#467
Open
Jbrandan wants to merge 5 commits into
Open
feat: Implement Phase 31 Data and Persistence manage_data tool#467Jbrandan wants to merge 5 commits into
Jbrandan wants to merge 5 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new manage_data MCP tool: Unreal-side ChangesManage Data Tool Implementation
Review Status & Meta-Artifacts
Sequence DiagramsThe manage_data handler processes requests through a branching subAction dispatcher that routes to config (GConfig), save-game (UGameplayStatics), or placeholder operations, with validation and error handling at each step. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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 |
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/definitions/shared/all-tool-definitions.ts (1)
27-52:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCanonical tool registry count contract is violated.
Adding
manageDataToolDefinitionat Line 51 increasesallToolDefinitionsto 24 entries, but this file’s contract requires exactly 23 definitions in public list order. Please either keep the canonical list at 23 or update the canonical-count contract and all dependent audits/registrations together before merge.As per coding guidelines,
src/tools/**/definitions/shared/all-tool-definitions.tsmust maintain exactly 23 definitions in public list order.🤖 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 `@src/tools/definitions/shared/all-tool-definitions.ts` around lines 27 - 52, The public canonical registry allToolDefinitions now has 24 entries due to the added manageDataToolDefinition which violates the required 23-definition contract; either remove manageDataToolDefinition from the exported allToolDefinitions array to restore the 23-entry canonical list (preserving the public list order), or if manageDataToolDefinition must remain, update the canonical-count contract and all dependent audits/registrations together (search for uses of allToolDefinitions, registry/audit code and any canonical-count constants) so the canonical list, ordering and downstream validations are updated consistently.Source: Coding guidelines
🧹 Nitpick comments (1)
src/tools/orchestration/consolidated-routing.ts (1)
76-87: ⚡ Quick winConsolidate
manage_dataaction literals into one shared source.
dataActionSetduplicates the full action enum that is also declared insrc/tools/definitions/gameplay/manage-data-tool.ts(Line 13-24). Keeping two hand-maintained lists risks silent routing misses (thedataActionSet.has(action)gate in handler registration can reject schema-valid actions if the lists drift). Prefer exporting one sharedMANAGE_DATA_ACTIONSconstant and deriving both the schema enum and this set from it.As per coding guidelines, tool schemas/action enums under
src/tools/{catalog,definitions}/**/*.tsare the source of truth and should not diverge from runtime routing lists.🤖 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 `@src/tools/orchestration/consolidated-routing.ts` around lines 76 - 87, Replace the duplicated literal list with a single shared constant: export a MANAGE_DATA_ACTIONS array/readonly tuple from the manage-data-tool declaration (the source-of-truth currently defining the action enum) and then import that constant into consolidated-routing.ts and build dataActionSet = new Set(MANAGE_DATA_ACTIONS); remove the hard-coded strings from dataActionSet, ensure any schema/enum definitions are derived from MANAGE_DATA_ACTIONS (or kept in sync by deriving the enum from that constant), and update imports and types so usages of dataActionSet and the schema still compile.Source: Coding guidelines
🤖 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
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp`:
- Around line 30-68: The handler is passing client-controlled configFilename
(via Payload->TryGetStringField) directly into
GConfig->GetString/SetString/Flush; update the SubAction branches
("read_config_value", "write_config_value", "flush_config") to validate and
constrain the filename before calling GConfig: require TryGetStringField to
return true (send an automation error if missing), normalize/sanitize the value
(reject empty, absolute paths, path separators, or ".."), map allowed inputs to
a whitelist of approved targets (e.g., "DefaultGame", "Game", "Engine" ->
resolve to GGameIni/GEngineIni constants) and default to the intended project
config only; only then call GConfig->GetString/SetString/Flush with the resolved
safe filename and use Subsystem->SendAutomationError on invalid inputs.
- Around line 111-119: The handler currently returns success for unimplemented
manage_data subActions (checks like SubAction.StartsWith(TEXT("create_data_"))
or TEXT("create_curve_")) and in the default branch using
Subsystem->SendAutomationResponse; change those branches to call
Subsystem->SendAutomationError(RequestingSocket, RequestId,
TEXT("UNSUPPORTED_ACTION"), ResultJson) so unsupported subActions return an
error until real implementations exist; update both the specific
create_data_/create_curve_ branches and the final default handler to use
SendAutomationError instead of SendAutomationResponse and keep RequestingSocket,
RequestId and ResultJson as the parameters.
- Around line 99-107: The current create_gameplay_tag branch uses
UGameplayTagsManager::Get().AddNativeGameplayTag(...) which only registers the
tag in-memory and does not persist to project config; update the handler (the
SubAction == "create_gameplay_tag" block that reads tagName/tagComment via
Payload->TryGetStringField and currently calls AddNativeGameplayTag, then
Subsystem->SendAutomationResponse) to either (A) persist the tag to a
config-backed gameplay tag source using the engine's gameplay tag source/config
APIs before sending a success response, or (B) reject the request with
SendAutomationResponse(false, ...) and a clear message stating persistence is
not implemented; ensure the code references the same TagName/TagComment values
and does not report "Gameplay tag created successfully." unless persistence is
actually completed.
In `@src/tools/handlers/data/data-handlers.ts`:
- Line 6: The function signature for handleDataTools uses a loose Promise<any>;
change its return type to Promise<unknown> to match the dispatch
executeAutomationRequest and keep the handler contract strict—update the
exported function declaration export async function handleDataTools(action:
string, args: HandlerArgs, tools: ITools): Promise<any> to use Promise<unknown>
and ensure any local references to the return type (if any) are adjusted
accordingly.
In `@src/tools/orchestration/consolidated-handler-registration.ts`:
- Around line 218-222: The current routing in
toolRegistry.register('manage_data') uses dataActionSet.has(action) but both
branches call handleDataTools, making the check a no-op; change the logic so
unknown actions are rejected instead of dispatched: after obtaining action via
getToolAction(args), if !dataActionSet.has(action) throw a clear Error (or
return a rejected Promise) that includes the domain "manage_data" and the
offending action; only call handleDataTools(action, args, tools) when the action
is present in dataActionSet.
In `@tests/mcp-tools/gameplay/manage-data.test.mjs`:
- Around line 45-49: The test case for the manage_data tool's delete_save_slot
scenario uses a broad expectation mask ('success|error') which masks
regressions; update the scenario object (the entry with scenario: 'SAVE:
delete_save_slot (not found)', toolName: 'manage_data', arguments: { action:
'delete_save_slot', slotName: SLOT_NAME, userIndex: 0 }) to use a single primary
expectation and an explicit allowed alternative (for example 'error:not found'
with an allowed alternative like 'success:not found' if your implementation may
return success), replacing 'success|error' with a concrete expected string that
documents the primary expected outcome and the acceptable alternative.
---
Outside diff comments:
In `@src/tools/definitions/shared/all-tool-definitions.ts`:
- Around line 27-52: The public canonical registry allToolDefinitions now has 24
entries due to the added manageDataToolDefinition which violates the required
23-definition contract; either remove manageDataToolDefinition from the exported
allToolDefinitions array to restore the 23-entry canonical list (preserving the
public list order), or if manageDataToolDefinition must remain, update the
canonical-count contract and all dependent audits/registrations together (search
for uses of allToolDefinitions, registry/audit code and any canonical-count
constants) so the canonical list, ordering and downstream validations are
updated consistently.
---
Nitpick comments:
In `@src/tools/orchestration/consolidated-routing.ts`:
- Around line 76-87: Replace the duplicated literal list with a single shared
constant: export a MANAGE_DATA_ACTIONS array/readonly tuple from the
manage-data-tool declaration (the source-of-truth currently defining the action
enum) and then import that constant into consolidated-routing.ts and build
dataActionSet = new Set(MANAGE_DATA_ACTIONS); remove the hard-coded strings from
dataActionSet, ensure any schema/enum definitions are derived from
MANAGE_DATA_ACTIONS (or kept in sync by deriving the enum from that constant),
and update imports and types so usages of dataActionSet and the schema still
compile.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 186b82a3-7e6c-40f9-b373-95b39298a5f0
📒 Files selected for processing (9)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Core/Subsystem/McpAutomationBridgeSubsystemHandlerRegistration.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cppplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.hsrc/tools/definitions/gameplay/manage-data-tool.tssrc/tools/definitions/shared/all-tool-definitions.tssrc/tools/handlers/data/data-handlers.tssrc/tools/orchestration/consolidated-handler-registration.tssrc/tools/orchestration/consolidated-routing.tstests/mcp-tools/gameplay/manage-data.test.mjs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/smoke-test.ts (1)
18-18: 📐 Maintainability & Code Quality | ⚡ Quick winConsider relaxing the hardcoded tool count check.
Using
z.literal(24)forces an exact match and couples this smoke test to the precise tool count. Every tool addition or removal will require updating this test, even though the smoke test's purpose is to verify basic system health (server starts, responds correctly), not to validate specific tool counts.♻️ Proposed refactor to use a minimum-count assertion
- totalTools: z.literal(24) + totalTools: z.number().int().gte(24)This allows the test to pass as long as at least 24 tools are present, reducing maintenance burden while still catching regressions where tools disappear unexpectedly.
🤖 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 `@scripts/smoke-test.ts` at line 18, The schema currently uses a strict equality for totalTools (totalTools: z.literal(24)) which breaks whenever tools are added/removed; change it to a minimum-count assertion such as totalTools: z.number().int().min(24) (or z.number().gte(24)) so the smoke test verifies at least 24 tools exist instead of exact equality, updating the schema where totalTools is defined.
🤖 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.
Nitpick comments:
In `@scripts/smoke-test.ts`:
- Line 18: The schema currently uses a strict equality for totalTools
(totalTools: z.literal(24)) which breaks whenever tools are added/removed;
change it to a minimum-count assertion such as totalTools:
z.number().int().min(24) (or z.number().gte(24)) so the smoke test verifies at
least 24 tools exist instead of exact equality, updating the schema where
totalTools is defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec54c9a5-5a6d-4455-a40d-7386fa0f4de4
📒 Files selected for processing (2)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cppscripts/smoke-test.ts
💤 Files with no reviewable changes (1)
- plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp
15 tasks
Owner
|
Hey! Thanks for adding this — Blocking:
Worth fixing:
Nits:
Overall, good start — I just don’t want us to merge a tool that advertises more support than it actually has yet. |
- Trimmed manage-data tool schema to only expose implemented actions. - Added strict payload validation for config file and save slot operations. - Restricted configFilename to a whitelist of allowed Engine/Project .ini files. - Fixed implementaton typo in McpAutomationBridge_DataHandlers.cpp. - Updated create_gameplay_tag and mocked data asset actions to return NOT_IMPLEMENTED error. - Enforced dataActionSet strict validation in TS registry. - Updated docs to reflect the new 24 canonical tools count.
Contributor
📏 Large PR DetectedThis pull request is quite large (1000+ lines changed), which can make reviewing challenging. Suggestions:
This helps reviewers provide better feedback and speeds up the merge process. Thank you! 🙏 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp (1)
117-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault handler should reject unknown subActions, not succeed.
Line 117 returns
SendAutomationResponse(…, true, TEXT("Data action executed."), …)for anysubActionthat does not match an explicit branch. This means unrecognized or misspelled actions silently succeed instead of failing fast with a clear "unknown action" error, which can mislead callers and mask bugs.Replace the default success response with
SendAutomationError(…, FString::Printf(TEXT("Unknown manage_data subAction '%s'."), *SubAction), TEXT("UNKNOWN_ACTION")).🐛 Proposed fix
- // Default handler - Subsystem->SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Data action executed."), ResultJson); + Subsystem->SendAutomationError( + RequestingSocket, + RequestId, + FString::Printf(TEXT("Unknown manage_data subAction '%s'."), *SubAction), + TEXT("UNKNOWN_ACTION")); return true;🤖 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 `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` around lines 117 - 118, The default case for unknown subActions is incorrectly succeeding instead of failing. Replace the SendAutomationResponse call at lines 117-118 with a call to SendAutomationError that rejects unknown actions with a clear error message. Specifically, replace the success response (which has true as the success parameter and "Data action executed." as the message) with SendAutomationError passing RequestingSocket and RequestId, a formatted error message using FString::Printf to include the actual SubAction value (e.g., "Unknown manage_data subAction '%s'."), and the error code "UNKNOWN_ACTION". This ensures misspelled or unrecognized actions fail fast and clearly rather than silently succeeding.
♻️ Duplicate comments (2)
src/tools/handlers/data/data-handlers.ts (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace
Promise<any>withPromise<unknown>to maintain strict typing.Line 6 declares
handleDataToolswith return typePromise<any>, but the underlyingexecuteAutomationRequestdispatch returnsPromise<unknown>(see context snippet 1, line 65). Usinganyweakens the type contract and bypasses TypeScript's strict checks.Change the return type to
Promise<unknown>to align with the dispatch contract and project guidelines for strict NodeNext TypeScript.🛡️ Proposed fix
-export async function handleDataTools(action: string, args: HandlerArgs, tools: ITools): Promise<any> { +export async function handleDataTools(action: string, args: HandlerArgs, tools: ITools): Promise<unknown> {🤖 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 `@src/tools/handlers/data/data-handlers.ts` at line 6, The handleDataTools function in data-handlers.ts has a return type of Promise<any> which weakens TypeScript's type safety, whereas the underlying executeAutomationRequest dispatch returns Promise<unknown>. Change the return type annotation of the handleDataTools function from Promise<any> to Promise<unknown> to maintain strict typing and align with the project's TypeScript guidelines and the dispatch contract.Source: Coding guidelines
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp (1)
28-68:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftConfig operations still lack required-field validation and filename whitelisting.
Lines 30-33, 49-53, and 63-64 ignore the return values from
TryGetStringField, allowing empty or missingconfigFilename,configSection,configKey, andconfigValueto pass silently toGConfig. This creates two hazards: (1) missing required parameters produce confusing behavior instead of clear errors, and (2) arbitrary client-controlled.inipaths reachGConfig->GetString/SetString/Flushwithout any whitelist constraint, turning this into a general config-file accessor instead of the narrow project-config operation the tool contract describes.Validate that all required fields are present and non-empty, then resolve
configFilenameagainst a whitelist (e.g., map user-friendly tokens like"Game"or"DefaultGame.ini"toGGameIni,"Engine"toGEngineIni) before callingGConfig.🤖 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 `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` around lines 28 - 68, The read_config_value, write_config_value, and flush_config handlers ignore the return values from TryGetStringField calls, allowing empty or missing required parameters (configFilename, configSection, configKey, configValue) to pass silently to GConfig operations. Additionally, configFilename is used directly without validation or whitelisting, allowing arbitrary client-controlled .ini file paths to reach GConfig. Fix this by: (1) checking the return value of each TryGetStringField call and validating that required fields are non-empty before proceeding—if validation fails, send an error response indicating the missing field; (2) implementing a whitelist mapping for configFilename that translates safe tokens like "Game" to GGameIni, "Engine" to GEngineIni, and rejects unmapped values with an error response, ensuring only intended config files are accessible through this handler.
🧹 Nitpick comments (3)
src/tools/definitions/gameplay/manage-data-tool.ts (1)
35-39: ⚡ Quick win
configFilenameenum mixes short names and full.inifilenames.Lines 35-39 define
configFilenamewith an enum that includes both short tokens ('Game','Engine','Input') and full filenames ('DefaultGame.ini','DefaultEngine.ini','DefaultInput.ini'). This creates ambiguity for callers and requires the C++ handler to normalize both forms to the same canonical target (e.g., map both"Game"and"DefaultGame.ini"toGGameIni).Consider keeping only the short canonical tokens (
'Game','Engine', etc.) in the enum and having the C++ handler map them to the appropriate constants, or document the preferred form in the description to guide callers.🤖 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 `@src/tools/definitions/gameplay/manage-data-tool.ts` around lines 35 - 39, The configFilename enum mixes both short canonical names (Game, Engine, Input) and full filenames (DefaultGame.ini, DefaultEngine.ini, DefaultInput.ini), creating ambiguity for callers. To fix this, keep only the short canonical tokens (Game, Engine, Input, GameUserSettings, EditorPerProjectUserSettings, Editor) in the enum for the configFilename property, removing the full .ini filename variants. The C++ handler will then be responsible for mapping these short tokens to their corresponding full filenames or constants, eliminating the need to handle both forms.plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp (2)
103-104: ⚡ Quick winUse
SendAutomationErrorinstead ofSendAutomationResponse(…, false, …)for NOT_IMPLEMENTED responses.Line 104 calls
SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), ResultJson)to signal that gameplay-tag persistence is not implemented. Semantically, failures and unsupported operations belong in the error channel; using the success-response method withsuccess: falseconflates the two response types and makes client-side error handling less predictable.Switch to
SendAutomationError(RequestingSocket, RequestId, TEXT("Persistence for gameplay tags is not yet implemented."), TEXT("NOT_IMPLEMENTED"))to keep error cases on the error path.♻️ Proposed fix
- ResultJson->SetStringField(TEXT("message"), TEXT("Persistence for gameplay tags is not yet implemented.")); - Subsystem->SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), ResultJson); + Subsystem->SendAutomationError( + RequestingSocket, + RequestId, + TEXT("Persistence for gameplay tags is not yet implemented."), + TEXT("NOT_IMPLEMENTED")); return true;🤖 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 `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` around lines 103 - 104, Replace the SendAutomationResponse call with SendAutomationError to properly signal unsupported operations through the error channel. Remove the ResultJson field setup and the SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), ResultJson) call, and instead use SendAutomationError(RequestingSocket, RequestId, TEXT("Persistence for gameplay tags is not yet implemented."), TEXT("NOT_IMPLEMENTED")) to handle the not-implemented case on the proper error path.
111-112: ⚡ Quick winUse
SendAutomationErrorfor data-asset NOT_IMPLEMENTED responses.Line 112 calls
SendAutomationResponse(…, false, TEXT("NOT_IMPLEMENTED"), …)to signal that data-asset creation is not implemented. As with the gameplay-tag case, unsupported operations should use the error channel for clarity.Switch to
SendAutomationError(RequestingSocket, RequestId, TEXT("Data asset creation is not yet implemented."), TEXT("NOT_IMPLEMENTED")).♻️ Proposed fix
- ResultJson->SetStringField(TEXT("message"), TEXT("Data asset creation is not yet implemented.")); - Subsystem->SendAutomationResponse(RequestingSocket, RequestId, false, TEXT("NOT_IMPLEMENTED"), ResultJson); + Subsystem->SendAutomationError( + RequestingSocket, + RequestId, + TEXT("Data asset creation is not yet implemented."), + TEXT("NOT_IMPLEMENTED")); return true;🤖 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 `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` around lines 111 - 112, The data-asset creation NOT_IMPLEMENTED response is using SendAutomationResponse with a false status instead of the dedicated error channel. Replace the SendAutomationResponse call with SendAutomationError(RequestingSocket, RequestId, TEXT("Data asset creation is not yet implemented."), TEXT("NOT_IMPLEMENTED")) and remove the unnecessary ResultJson->SetStringField line that sets the message field, since SendAutomationError handles the message parameter directly.
🤖 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 `@pr434_last.txt`:
- Line 1: The tracking metadata files contain corrupted replacement characters
that render them unreadable and may break downstream tooling. Replace the
corrupted content at pr434_last.txt (line 1-1) with the intended tracking value,
replace the corrupted content at pr467_last.txt (line 1-1) with the intended
tracking value, and replace the corrupted content at pr468_last.txt (line 1-1)
with the intended tracking value. Each file should contain valid plain text
status markers instead of the current unreadable replacement characters.
---
Outside diff comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp`:
- Around line 117-118: The default case for unknown subActions is incorrectly
succeeding instead of failing. Replace the SendAutomationResponse call at lines
117-118 with a call to SendAutomationError that rejects unknown actions with a
clear error message. Specifically, replace the success response (which has true
as the success parameter and "Data action executed." as the message) with
SendAutomationError passing RequestingSocket and RequestId, a formatted error
message using FString::Printf to include the actual SubAction value (e.g.,
"Unknown manage_data subAction '%s'."), and the error code "UNKNOWN_ACTION".
This ensures misspelled or unrecognized actions fail fast and clearly rather
than silently succeeding.
---
Duplicate comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp`:
- Around line 28-68: The read_config_value, write_config_value, and flush_config
handlers ignore the return values from TryGetStringField calls, allowing empty
or missing required parameters (configFilename, configSection, configKey,
configValue) to pass silently to GConfig operations. Additionally,
configFilename is used directly without validation or whitelisting, allowing
arbitrary client-controlled .ini file paths to reach GConfig. Fix this by: (1)
checking the return value of each TryGetStringField call and validating that
required fields are non-empty before proceeding—if validation fails, send an
error response indicating the missing field; (2) implementing a whitelist
mapping for configFilename that translates safe tokens like "Game" to GGameIni,
"Engine" to GEngineIni, and rejects unmapped values with an error response,
ensuring only intended config files are accessible through this handler.
In `@src/tools/handlers/data/data-handlers.ts`:
- Line 6: The handleDataTools function in data-handlers.ts has a return type of
Promise<any> which weakens TypeScript's type safety, whereas the underlying
executeAutomationRequest dispatch returns Promise<unknown>. Change the return
type annotation of the handleDataTools function from Promise<any> to
Promise<unknown> to maintain strict typing and align with the project's
TypeScript guidelines and the dispatch contract.
---
Nitpick comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp`:
- Around line 103-104: Replace the SendAutomationResponse call with
SendAutomationError to properly signal unsupported operations through the error
channel. Remove the ResultJson field setup and the
SendAutomationResponse(RequestingSocket, RequestId, false,
TEXT("NOT_IMPLEMENTED"), ResultJson) call, and instead use
SendAutomationError(RequestingSocket, RequestId, TEXT("Persistence for gameplay
tags is not yet implemented."), TEXT("NOT_IMPLEMENTED")) to handle the
not-implemented case on the proper error path.
- Around line 111-112: The data-asset creation NOT_IMPLEMENTED response is using
SendAutomationResponse with a false status instead of the dedicated error
channel. Replace the SendAutomationResponse call with
SendAutomationError(RequestingSocket, RequestId, TEXT("Data asset creation is
not yet implemented."), TEXT("NOT_IMPLEMENTED")) and remove the unnecessary
ResultJson->SetStringField line that sets the message field, since
SendAutomationError handles the message parameter directly.
In `@src/tools/definitions/gameplay/manage-data-tool.ts`:
- Around line 35-39: The configFilename enum mixes both short canonical names
(Game, Engine, Input) and full filenames (DefaultGame.ini, DefaultEngine.ini,
DefaultInput.ini), creating ambiguity for callers. To fix this, keep only the
short canonical tokens (Game, Engine, Input, GameUserSettings,
EditorPerProjectUserSettings, Editor) in the enum for the configFilename
property, removing the full .ini filename variants. The C++ handler will then be
responsible for mapping these short tokens to their corresponding full filenames
or constants, eliminating the need to handle both forms.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2047d511-5d92-4a45-9fe6-d5bbef4e30fc
📒 Files selected for processing (24)
AGENTS.mdplugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpppr434_body.jsonpr434_comments.jsonpr434_last.txtpr434_reviews.jsonpr434_reviews_full.jsonpr434_reviews_summary.txtpr467_comments.jsonpr467_last.txtpr467_reviews.jsonpr467_reviews_summary.txtpr468_comments.jsonpr468_last.txtpr468_review.mdpr468_reviews.jsonpr468_reviews_summary.txtpull_request_phase_34.mdrecent_comments.jsonrecent_reviews.jsonsrc/tools/definitions/gameplay/manage-data-tool.tssrc/tools/handlers/data/data-handlers.tssrc/tools/orchestration/consolidated-handler-registration.tstests/mcp-tools/gameplay/manage-data.test.mjs
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
| @@ -0,0 +1 @@ | |||
| �� No newline at end of file | |||
Contributor
There was a problem hiding this comment.
Regenerate the tracking files with valid text.
The �� replacement characters make these metadata files unreadable and may break downstream tooling that expects plain text status markers.
pr434_last.txt#L1-L1: replace the corrupted line with the intended tracking value.pr467_last.txt#L1-L1: replace the corrupted line with the intended tracking value.pr468_last.txt#L1-L1: replace the corrupted line with the intended tracking value.
📍 Affects 3 files
pr434_last.txt#L1-L1(this comment)pr467_last.txt#L1-L1pr468_last.txt#L1-L1
🤖 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 `@pr434_last.txt` at line 1, The tracking metadata files contain corrupted
replacement characters that render them unreadable and may break downstream
tooling. Replace the corrupted content at pr434_last.txt (line 1-1) with the
intended tracking value, replace the corrupted content at pr467_last.txt (line
1-1) with the intended tracking value, and replace the corrupted content at
pr468_last.txt (line 1-1) with the intended tracking value. Each file should
contain valid plain text status markers instead of the current unreadable
replacement characters.
- Changed SendAutomationResponse to SendAutomationError for missing config parameters, NOT_IMPLEMENTED states, and default UNKNOWN_ACTION. - Validated required config payload parameters and mapped valid configFilenames to safe GConfig globals. - Changed handleDataTools return type to Promise<unknown>. - Stripped full .ini filenames from manage_data tool schema configFilename enum. - Fixed text corruption in PR tracking files.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp (1)
104-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
slotNamebefore calling save-slot operations.The
check_save_slot_existsanddelete_save_slotbranches (lines 108, 120) callTryGetStringField("slotName", SlotName)but ignore the return value. If the field is missing or not a string,SlotNameremains an emptyFString, which is then passed toUGameplayStatics::DoesSaveGameExist/DeleteGameInSlot. This creates two issues:
- Contract mismatch: The TypeScript handler in
data-handlers.tsrequiresslotNameviarequireNonEmptyStringfor both actions, but the C++ silently accepts missing/empty values.- Inconsistency: The config branches (lines 41-48, 67-75, 88-93) validate all required parameters and return
MISSING_PARAMETERerrors, but the save-slot branches do not.Validate that
slotNameis present and non-empty, and return aMISSING_PARAMETERerror if it's missing, consistent with the config handlers and the TypeScript-side contract.🛡️ Proposed fix to add slotName validation
else if (SubAction.Equals(TEXT("check_save_slot_exists"))) { FString SlotName; double UserIndex = 0; - Payload->TryGetStringField(TEXT("slotName"), SlotName); + if (!Payload->TryGetStringField(TEXT("slotName"), SlotName) || SlotName.IsEmpty()) + { + Subsystem->SendAutomationError(RequestingSocket, RequestId, TEXT("Missing or empty required parameter (slotName)."), TEXT("MISSING_PARAMETER")); + return true; + } Payload->TryGetNumberField(TEXT("userIndex"), UserIndex); bool bExists = UGameplayStatics::DoesSaveGameExist(SlotName, (int32)UserIndex); ResultJson->SetBoolField(TEXT("exists"), bExists); Subsystem->SendAutomationResponse(RequestingSocket, RequestId, true, TEXT("Save slot check complete."), ResultJson); return true; } else if (SubAction.Equals(TEXT("delete_save_slot"))) { FString SlotName; double UserIndex = 0; - Payload->TryGetStringField(TEXT("slotName"), SlotName); + if (!Payload->TryGetStringField(TEXT("slotName"), SlotName) || SlotName.IsEmpty()) + { + Subsystem->SendAutomationError(RequestingSocket, RequestId, TEXT("Missing or empty required parameter (slotName)."), TEXT("MISSING_PARAMETER")); + return true; + } Payload->TryGetNumberField(TEXT("userIndex"), UserIndex); bool bDeleted = UGameplayStatics::DeleteGameInSlot(SlotName, (int32)UserIndex);🤖 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 `@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp` around lines 104 - 127, Validate the slotName parameter before performing save-slot operations in both the check_save_slot_exists and delete_save_slot branches. Check the return value of TryGetStringField when retrieving "slotName" and verify that the resulting SlotName is not empty. If validation fails, send an automation response with a MISSING_PARAMETER error (following the pattern used in the config handlers like the ones for set_config and delete_config) instead of proceeding to call UGameplayStatics::DoesSaveGameExist or UGameplayStatics::DeleteGameInSlot with an empty slot name. This ensures consistency with both the TypeScript-side contract (which requires non-empty slotName) and the existing parameter validation pattern used throughout the config handler branches.
🤖 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.
Outside diff comments:
In
`@plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpp`:
- Around line 104-127: Validate the slotName parameter before performing
save-slot operations in both the check_save_slot_exists and delete_save_slot
branches. Check the return value of TryGetStringField when retrieving "slotName"
and verify that the resulting SlotName is not empty. If validation fails, send
an automation response with a MISSING_PARAMETER error (following the pattern
used in the config handlers like the ones for set_config and delete_config)
instead of proceeding to call UGameplayStatics::DoesSaveGameExist or
UGameplayStatics::DeleteGameInSlot with an empty slot name. This ensures
consistency with both the TypeScript-side contract (which requires non-empty
slotName) and the existing parameter validation pattern used throughout the
config handler branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f779ab9e-baee-45e2-831c-a7f5e988c9c7
📒 Files selected for processing (6)
plugins/McpAutomationBridge/Source/McpAutomationBridge/Private/Domains/Data/McpAutomationBridge_DataHandlers.cpppr434_last.txtpr467_last.txtpr468_last.txtsrc/tools/definitions/gameplay/manage-data-tool.tssrc/tools/handlers/data/data-handlers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tools/definitions/gameplay/manage-data-tool.ts
- src/tools/handlers/data/data-handlers.ts
…e_save_slot - Added validation for slotName to ensure it is provided and not empty before calling GamePlayStatics functions. - This aligns with config handler parameter validation and TS expectations.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 implements Phase 31: Data & Persistence from the project Roadmap. It introduces a new canonical tool
manage_datato handle operations related to Data Assets, Save Systems, Gameplay Tags, and Configs, aligning with the consolidated 22+ tools strategy from Phase 53.Changes
manage-data-tool.tsexposing over 25+ subActions for data & persistence.data-handlers.tsto forwardmanage_datacommands with context validation.manage_dataintoall-tool-definitions.ts,consolidated-routing.tsandconsolidated-handler-registration.ts.McpAutomationBridge_DataHandlers.hand.cppcovering implementation forUGameplayStatics,GConfigandUGameplayTagsManager.McpAutomationBridgeSubsystemHandlerRegistration.cpp.manage-data.test.mjsto ensure the TypeScript side successfully parses and issues commands.Related Issues
Resolves Phase 31: Data & Persistence
Type of Change
Testing
Test Runner)Pre-Merge Checklist