terrymanu commented on PR #39169: URL: https://github.com/apache/shardingsphere/pull/39169#issuecomment-5002824014
### Summary **Review Result: Mergeable** **Reason:** This result applies only to the latest PR’s code scope; CI was not reviewed by request. The changes fix the root cause of false positives in the sensitive field-name heuristic while preserving fail-closed safety boundaries. No code-level blockers were found. ### Evidence - `mcp/bootstrap/.../MCPToolClarificationPolicy.java:105` removes broad field-name matching, including markers such as `key`, and relies on explicit `secret` metadata from the producer. Form elicitation is still rejected when the metadata is missing or malformed, or when `input_type=secret`. - `mcp/support/.../WorkflowGuidancePayloadBuilder.java:167` is the repository’s only production path for creating clarification questions. Known algorithm properties use `AlgorithmPropertyRequirement.secret`, while unknown algorithm properties default to sensitive. - `mcp/bootstrap/.../MCPToolElicitationFlowTest.java:86` covers the complete `key_generator` flow from clarification through the continued planning-tool call. The previous implementation incorrectly blocked this path because of the `key` marker. - `mcp/support/.../WorkflowPlanPayloadBuilderTest.java:104` covers both known non-sensitive properties and fail-closed handling for unknown properties. - This behavior complies with MCP 2025-11-25, which prohibits requesting passwords, API keys, access tokens, and other sensitive information through form-mode elicitation. [MCP Elicitation specification](https://modelcontextprotocol.io/specification/2025-11-25/client/elicitation) - `mcp/bootstrap/.../SessionManagedStdioTransportProviderTest.java:157` replaces and restores `System.in` only while constructing the test provider. MCP Java SDK 1.1.2 captures the input stream during construction, allowing the test to receive immediate EOF without changing the production construction path. [MCP Java SDK 1.1.2 source](https://github.com/modelcontextprotocol/java-sdk/blob/v1.1.2/mcp-core/src/main/java/io/modelcontextprotocol/server/transport/StdioServerTransportProvider.java) - `RuleArtifact.toMap()` has no repository callers and bypasses the centralized `WorkflowArtifactMaskUtils` serialization path. The MCP module was introduced after the 5.5.3 release, so removing this method does not create a compatibility issue for a released artifact. ### Review Details - **Review Focus:** Code Correctness Review — CI not reviewed by request. - **Reviewed Scope:** All 9 MCP files changed by the PR. The latest head is `57a46c75cdd42490cfd7b803a35d423b70a4b84c`, the base is `dad38461489d1def0d4ff5183e5f37b2a9b6c6b2`, and the local merge base is `0c018851c18c66114aecbd9c4be2697e98179cea`. The local triple-dot file list exactly matches GitHub’s `/pulls/39169/files` response. - **Not Reviewed Scope:** GitHub Actions, check runs, and non-MCP functional behavior. - **Verification:** - `./mvnw -f mcp/pom.xml test -Pcheck -T1C` — exit `0`, `BUILD SUCCESS`; all 290 bootstrap tests passed without the previous fork-JVM shutdown delay. - `./mvnw checkstyle:check -Pcheck -T1C` — exit `0`, 0 violations. - `./mvnw spotless:check -Pcheck -T1C` — exit `0`, `BUILD SUCCESS`. - **Release Note / User Docs:** Not required. The PR changes internal MCP clarification safety and test lifecycle behavior without introducing configuration, migration, dependency, or released API requirements. -- 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]
