terrymanu commented on PR #39024:
URL: https://github.com/apache/shardingsphere/pull/39024#issuecomment-4899911355
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. CI not reviewed by request. The latest PR
head removes runnable-distribution placeholder config, preserves an explicit
empty-runtime-database startup state, adds Docker-specific HTTP defaults, and
validates the new streamable-http registry runtime metadata without introducing
a confirmed code-level blocker.
### Evidence
- Distribution defaults now use real startup config instead of unresolved
runtime database placeholders:
`distribution/mcp/src/main/resources/conf/mcp-http.yaml:24`,
`distribution/mcp/src/main/resources/conf/mcp-stdio.yaml:19`, and Docker HTTP
uses `distribution/mcp/src/main/resources/conf/mcp-http-docker.yaml:18`.
- Docker startup now selects the Docker-specific HTTP config when
`SHARDINGSPHERE_MCP_TRANSPORT` is `http`, while still honoring explicit args
and `SHARDINGSPHERE_MCP_CONFIG`:
`distribution/mcp/src/main/bin/docker-entrypoint.sh:28`.
- YAML validation keeps `runtimeDatabases` non-null at the launch-config
boundary, allows empty maps, and rejects unresolved placeholder database
names/properties before runtime conversion:
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/config/YamlMCPLaunchConfiguration.java:42`,
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/validator/YamlRuntimeDatabaseConfigurationsValidator.java:44`,
and
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/validator/YamlRuntimeDatabaseConfigurationsValidator.java:82`.
- The new empty and placeholder cases are covered directly in the YAML
swapper tests:
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/swapper/YamlMCPLaunchConfigurationSwapperTest.java:115`,
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/swapper/YamlMCPLaunchConfigurationSwapperTest.java:133`,
and
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/config/yaml/swapper/YamlMCPLaunchConfigurationSwapperTest.java:235`.
- `mcp/server.json` now declares streamable-http Docker runtime metadata at
package level, which matches the official MCP server schema fields for
`Package.runtimeHint` and `Package.runtimeArguments`: `mcp/server.json:43` and
`mcp/server.json:48`.
- Registry validation protects the new streamable-http Docker metadata from
drifting away from the documented localhost port binding:
`mcp/registry/src/main/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommand.java:191`
and
`mcp/registry/src/main/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommand.java:200`.
- Source metadata validation and negative cases are covered by registry
tests:
`mcp/registry/src/test/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommandTest.java:87`,
`mcp/registry/src/test/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommandTest.java:97`,
and
`mcp/registry/src/test/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommandTest.java:166`.
- The E2E workflow now includes distribution changes in its PR path trigger:
`.github/workflows/e2e-mcp.yml:28`.
- User docs now explain empty startup config, placeholder rejection, and
Docker defaults:
`docs/document/content/user-manual/shardingsphere-mcp/configuration.en.md:65`,
`docs/document/content/user-manual/shardingsphere-mcp/configuration.en.md:96`,
`docs/document/content/user-manual/shardingsphere-mcp/deployment.en.md:34`, and
`docs/document/content/user-manual/shardingsphere-mcp/troubleshooting.en.md:29`.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39024 latest head
`3c20beec95d5b0f2a1e78ae57e605f4dcafe9475`, base `master` SHA
`3dbbca8dd881cf64e84c9ec5908d97c1524109e1`, local merge-base
`53049dfcd2c2524af11128d3d2864aea6424777d`. Reviewed all 20 GitHub-listed files
across `.github/workflows/e2e-mcp.yml`, `distribution/mcp`, `mcp/bootstrap`,
`mcp/registry`, `mcp/server.json`, and ShardingSphere-MCP user docs. Local
triple-dot file list matched GitHub `/pulls/39024/files`.
- Not Reviewed Scope: GitHub Actions/check-runs and Actions logs were not
queried. Live Docker image startup and full MCP LLM E2E execution were not run;
the review used source, tests, schema evidence, package build, and packaged
artifact inspection.
- Verification:
- `./mvnw spotless:check -Pcheck -T1C` exit 0, `BUILD SUCCESS`.
- `./mvnw checkstyle:check -Pcheck -T1C` exit 0, `BUILD SUCCESS`.
- `./mvnw -pl mcp/bootstrap,mcp/registry -am test -DskipITs
-Dspotless.skip=true -Dsurefire.failIfNoSpecifiedTests=false -B -ntp` exit 0,
`mcp/bootstrap` tests run 278 and `mcp/registry` tests run 24, `BUILD SUCCESS`.
- `./mvnw -pl distribution/mcp -am -DskipTests package -B -ntp` exit 0,
`BUILD SUCCESS`; packaged artifact contains `conf/mcp-http-docker.yaml`, empty
`runtimeDatabases` defaults, and Docker entrypoint reference to
`mcp-http-docker.yaml`.
- `./mvnw -pl test/e2e/mcp -am install -DskipTests -DskipITs
-Dspotless.skip=true -B -ntp` exit 0, `BUILD SUCCESS`.
- Official MCP schema check confirmed package-level `runtimeHint` and
`runtimeArguments` definitions.
- Release Note / User Docs: User docs are required for this user-visible
configuration/default behavior and were updated in both English and Chinese. A
separate release-note entry is not required for this code-scope review because
the PR documents the MCP configuration/deployment behavior directly.
--
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]
