terrymanu commented on PR #38748: URL: https://github.com/apache/shardingsphere/pull/38748#issuecomment-4569680505
### Decision - **Merge Verdict: Mergeable** - **Reviewed Scope:** Latest PR head `383de37032dee2dd1ed570cae3e5c864690e2319`; local merge-base `275c84a0a024045ec990c3b7f325b2f26d9b25b8`; local triple-dot file list matched GitHub `/pulls/38748/files`. Reviewed the 9 changed files under `mcp/core` and `mcp/support`, including `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPRuntimeProtectionPolicy.java`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPClientSafetyPolicy.java`, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/SQLExecutionToolHandlerSupport.java`, and `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/resource/handler/capability/RuntimeStatusHandler.java`. - **Not Reviewed Scope:** Full repository build, MCP E2E/runtime production tests, broader issue `#35294` design items outside this PR, and GitHub Actions/check-run status. - **Need Expert Review:** No additional expert review required. This PR does not change parser grammar, SQL dialect behavior, dependencies, authentication enforcement, registry state, or Proxy/JDBC high-frequency DML/DQL execution paths. ### Basis - The change directly addresses the metadata/source-of-truth gap by moving MCP runtime protection constants into `MCPRuntimeProtectionPolicy`, then reusing them from SQL execution argument validation. The effective `max_rows` and `timeout_ms` defaults and bounds remain unchanged at `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPRuntimeProtectionPolicy.java:34`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPRuntimeProtectionPolicy.java:36`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPRuntimeProtectionPolicy.java:38`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPRuntimeProtectionPolicy.java:40`, and `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/SQLExecutionToolHandlerSupport.java:71`. - Backward compatibility is preserved: `MCPClientSafetyPolicy` keeps the existing public constants and getter while delegating to the new policy class at `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPClientSafetyPolicy.java:32`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPClientSafetyPolicy.java:34`, and `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPClientSafetyPolicy.java:41`. - The new model-facing and runtime payload exposure is additive: the existing `tool_call_limit` field remains, and the grouped `runtime_protection` payload is added at `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/security/MCPClientSafetyPolicy.java:57` and `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/resource/handler/capability/RuntimeStatusHandler.java:71`. - The new public production type has direct tests in `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/security/MCPRuntimeProtectionPolicyTest.java:31`, including configured, non-positive, and missing system-property paths plus payload shape coverage. - The changed tests follow the relevant project rules I checked from `CODE_OF_CONDUCT.md:8`, `CODE_OF_CONDUCT.md:11`, `CODE_OF_CONDUCT.md:18`, `CODE_OF_CONDUCT.md:19`, `CODE_OF_CONDUCT.md:95`, and `CODE_OF_CONDUCT.md:98`. ### Verification - GitHub file-list boundary matched local scope: 9 changed files, 269 additions, 36 deletions. - Ran `./mvnw -pl mcp/core,mcp/support -am -DskipITs -Dspotless.skip=true -Dtest=MCPRuntimeProtectionPolicyTest,MCPClientSafetyPolicyTest,RuntimeStatusHandlerTest,ServerCapabilitiesHandlerTest,MCPModelFirstContractPayloadBuilderTest -Dsurefire.failIfNoSpecifiedTests=false test`: exit code 0. - Ran `./mvnw -pl mcp/core,mcp/support -am spotless:check -Pcheck -T1C`: exit code 0. - Ran `./mvnw -pl mcp/core,mcp/support -am checkstyle:check -Pcheck -T1C`: exit code 0. - Fresh adversarial pass found no unresolved blocker: no parser/dialect family impact, no name-resolution/default-schema/fallback-order change, no dependency or supply-chain change, no new direct `ConcurrentHashMap#computeIfAbsent` in the PR diff, and the default/non-positive property paths remain covered. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
