terrymanu commented on PR #38831: URL: https://github.com/apache/shardingsphere/pull/38831#issuecomment-4648001313
### Summary - **Merge Decision: Mergeable** - **Reason:** The PR keeps the existing MCP Markdown loading behavior while reducing the license-header stripping logic to one scoped pattern, with matching focused test coverage. ### Evidence - Root-cause / intent evidence: `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoader.java:36` now uses one anchored `APACHE_LICENSE_HEADER_PATTERN` to remove only a leading HTML comment containing the Apache license text, and `removeApacheLicenseHeader(...)` is used before the existing final `.strip()` call. - Regression evidence: existing tests still cover the important behavior boundaries: Apache license header removal, fallback class loader usage, authored leading comment preservation, missing resource, unreadable resource, and license-only blank resource rejection in `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoaderTest.java:40`. - Scope / ownership evidence: the PR changes only `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoader.java`; no public API, dependency, configuration, SQL parser, metadata, routing, or shared execution semantics are changed. - Compatibility evidence: loader callers remain unchanged through the same public `load(...)` method used by prompt templates and server instructions; the regex is anchored to the beginning of content and constrained within the first HTML comment, so authored comments are not newly removed. - Risk scan: no `ConcurrentHashMap#computeIfAbsent`, SQL dialect behavior, parser behavior, feature-disabled path, diagnostics, distribution, dependency, or release artifact changes were introduced. ### Review Details - **Reviewed Scope:** PR latest head `f0139075f44f7568c301b12e19bde0d55c7f1eb4`; base and local merge-base `3ea3c1bdecebe7a640c5ce5f74831521a6cd9b7e`; reviewed `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoader.java`. Local `git diff --name-status 3ea3c1bdecebe7a640c5ce5f74831521a6cd9b7e..refs/remotes/apache/pr/38831` matched GitHub `/pulls/38831/files`. - **Not Reviewed Scope:** CI/check-run status by design; full repository test suite; runtime MCP end-to-end behavior beyond the unchanged loader contract. - **Verification:** `./mvnw -pl mcp/support -am -DskipITs -Dspotless.skip=true -Dtest=MCPMarkdownResourceLoaderTest -Dsurefire.failIfNoSpecifiedTests=false test` passed with exit code 0; `./mvnw -pl mcp/support -am checkstyle:check -Pcheck -T1C` passed with exit code 0; `./mvnw -pl mcp/support -am spotless:check -Pcheck -T1C` passed with exit code 0. - **Release Note / User Docs:** Not required; this is an internal refactor that preserves existing MCP Markdown loader behavior and does not change user-facing configuration, protocol, SQL behavior, or documented usage. -- 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]
