terrymanu commented on PR #39215: URL: https://github.com/apache/shardingsphere/pull/39215#issuecomment-5048577304
### Summary **Review Result: Mergeable** The local candidate for PR #39215 passes the code correctness review. The original schema/runtime contract mismatch has been fixed without introducing unnecessary abstractions or changing supported behavior. > **Important:** The four fixes are still uncommitted and unpushed. The public PR head does not contain them yet. ### Root-Cause Fix - Reject `$defs`, legacy `definitions`, and nested `$ref` at the descriptor catalog boundary because runtime argument validation cannot resolve schema references. - Publish only the inline input-schema subset that the runtime actually validates. - Remove dead definition-traversal code and artificial reference-schema test data. - Add focused regression tests proving unsupported schema references are rejected before publication. ### Changed Scope - `mcp/support`: descriptor validation and regression tests. - `mcp/bootstrap`: SDK schema mapping and corresponding test cleanup. - Four files changed: 27 additions and 30 deletions. - No new public APIs, dependencies, configuration options, compatibility layers, or architectural abstractions. ### Verification | Check | Result | |---|---| | Regression tests before the fix | 3 expected failures | | Focused support/bootstrap tests | 18 passed | | Spotless | Passed | | Checkstyle | Passed | | MCP/core/bootstrap/MCP E2E reactor tests | 502 tests, 0 failures, 113 environment-dependent tests skipped | | `ci-mcp.yml`-equivalent module installation | `BUILD SUCCESS` | | Full PR file coverage | 120 of 120 files reviewed | | Adversarial review passes | 3 passes, 0 new findings | | Review ledger validation | `OK` | ### Review Details - **Review Focus:** Code Correctness Review. - **PR Head:** `c6e589ea7f8325db07cb19ddecce518cf398bb5b` - **Merge Base:** `d8fe562514b1703d331660ba2718305f40b87b90` - **GitHub/local file-list match:** 120/120. - **CI:** CI not reviewed by request. - **Not Reviewed:** Docker, conformance, real-LLM external E2E, and GitHub Actions check runs. - **Release Notes/User Documentation:** Not required because this corrects an internal contract for an unmerged capability and introduces no user-visible configuration or migration change. - **Remaining Code Blockers:** None. -- 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]
