atiaomar1978-hub commented on PR #25203: URL: https://github.com/apache/camel/pull/25203#issuecomment-5187850512
_AI-generated comment on behalf of [atiaomar1978-hub](https://github.com/atiaomar1978-hub)_ ### Re-review follow-up — Croway refactor (commits `4ac36e7`, `00b8a873`) Thanks @Croway for the detailed guidance and for landing the enabler in #25329 (CAMEL-24353). This push implements all four points from your review comment. --- #### 1. Reuse `VertxMcpServerEngine` instead of hand-rolled JSON-RPC **Removed (~450 lines):** - `StreamableMcpJsonRpcEngine` - `EmbeddedMcpRequestHandler` SPI + `JbangEmbeddedMcpRequestHandler` - `ManagementHttpServer` MCP dispatch wiring - `McpConfigurationProperties` and related generated configurer/metadata **Added:** - `JbangDevMcpServer` — builds `McpServerTool`s from `ToolRegistry` / `ToolDescriptor`, delegates to `VertxMcpServerEngine` - `ToolMcpSchemas` — maps `ToolDescriptor` params → JSON Schema for `inputSchemaJson()` (1:1 with prior bridge) - `JbangDevMcpMainListener` — registers the dev MCP service after Main configure The endpoint now gets the full SDK-backed streamable transport (sessions, SSE, GET replay, DELETE, `tools/list_changed`) instead of the POST-only subset. --- #### 2. Drop `camel.mcp.*` — use `camel.management.mcp*` instead | Before | After | |--------|-------| | `camel.mcp.enabled` / `host` / `port` / `path` | **removed** | | — | `camel.management.mcpEnabled` / `camel.management.mcpPath` (on `HttpManagementServerConfigurationProperties`) | - `--mcp` / `camel.jbang.mcp` now sets `camel.management.mcpEnabled=true` plus management-server enablement (same pattern as other management flags in `Run.java`) - `BaseMainSupport#setMcpProperties` cross-wiring removed — core `camel-main` stays MCP-free (only the existing `McpServerFactory` hook) - Business MCP on the main server remains under `camel.server.mcp-*` (CAMEL-24308) --- #### 3. Module boundaries - New code lives in **`camel-jbang-core`** + existing **`camel-platform-http-main`** management config - `camel-jbang-core` depends on **`camel-mcp-server`** - No MCP protocol code left in `camel-main` beyond management config properties --- #### 4. Docs Updated: - `camel-jbang-run.adoc`, `camel-jbang-dev.adoc`, `camel-jbang-debug.adoc` - `camel-jbang-configuration.adoc` - `camel-4x-upgrade-guide-4_22.adoc` — documents the two MCP surfaces and cross-links `mcp-server.adoc`: - **Dev/diagnostics tools** — `<management-port>/mcp` via `--mcp` / `camel.management.mcpEnabled` - **Business `ai-tool` routes** — `<server-port>/mcp` via `camel.server.mcp-enabled=true` --- #### Bug fix: shared-port management fallback (`00b8a873`) When the management server reuses the main HTTP port, only a `server`-typed `VertxPlatformHttpRouter` is registered. `JbangDevMcpServer#resolveTargetServerType()` now prefers `management` when present, otherwise falls back to `server` — so MCP still mounts when management shares the main port. --- #### Tests | Test | Coverage | |------|----------| | `RunMcpOptionTest` | `--mcp` flag → `camel.management.mcpEnabled` + management enablement | | `JbangDevMcpServerTest` | Tool mapping, `resolveTargetServerType()` fallback | | `ToolMcpSchemasTest` | JSON Schema generation from tool params | --- #### Dependency on #25329 This branch uses `VertxMcpServerEngine#setTargetServerType(...)`. Once #25329 is merged, this PR should rebase cleanly onto `main`. The management-router registration path is already wired. Ready for another look when you have a moment. Happy to adjust anything that still feels off scope-wise. -- 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]
