CAMEL-23853: Embed MCP server on camel run/dev and Camel Main - #25203
CAMEL-23853: Embed MCP server on camel run/dev and Camel Main#25203atiaomar1978-hub wants to merge 10 commits into
Conversation
AI review summary (Bugbot + Grok)Review comments generated by Cursor on behalf of atiaomar1978-hub. Bugbot
Grok-style review
Follow-up commit: |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
CI follow-upAI-generated comment on behalf of atiaomar1978-hub Addressed the doc failures from run 30398071305 (Antora) and run 30398074689 (incremental build /
Please re-check CI when the new workflow run completes. |
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 66 tested, 25 compile-only — current: 62 all testedMaveniverse Scalpel detected 91 affected modules (current approach: 62).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Review: CAMEL-23853 — Embed MCP server on camel run/dev and Camel Main
This is a well-structured feature with clean separation of concerns: EmbeddedMcpRequestHandler as the SPI in camel-main, StreamableMcpJsonRpcEngine as a reusable JSON-RPC engine, and JbangEmbeddedMcpRequestHandler bridging to the JBang ToolRegistry. The security model is solid — @Metadata(security = "insecure:dev") on both camel.mcp.enabled and camel.management.mcpEnabled, management-server-only exposure, and Origin header CSRF protection all follow established patterns. The test suite and documentation are thorough. Nice work!
A few issues worth addressing:
1. MCP defaults silently override management server host/port
In BaseMainSupport.setMcpProperties(), the guards:
if (mcp.getHost() != null) {
management.setHost(mcp.getHost());
}
if (mcp.getPort() > 0) {
management.setPort(mcp.getPort());
}...are always true because McpConfigurationProperties has defaults of "127.0.0.1" (never null) and 8080 (always > 0). Since the management server's own default host is "0.0.0.0", enabling MCP silently narrows the bind address to localhost.
Mitigating factor: setHttpManagementServerProperties() runs after setMcpProperties(), so explicitly user-set camel.management.host properties would override MCP defaults. But users relying on the default 0.0.0.0 would be silently affected.
Suggested fix: Use null defaults for host and port in McpConfigurationProperties, and only propagate when explicitly set by the user.
2. Run.java overwrites mainListenerClasses without appending
In Run.java:
main.addInitialProperty("camel.main.mainListenerClasses",
"org.apache.camel.dsl.jbang.core.commands.mcp.EmbeddedMcpMainListener");This unconditionally sets the property, overwriting any existing value. Compare with KameletMain.java which correctly checks for existing listeners and appends with comma separation:
String listeners = getInitialProperties().getProperty("camel.main.mainListenerClasses");
if (listeners == null || listeners.isBlank()) {
addInitialProperty("camel.main.mainListenerClasses", embedded);
} else if (!listeners.contains(embedded)) {
addInitialProperty("camel.main.mainListenerClasses", listeners + "," + embedded);
}Currently harmless (no other listener is registered before the MCP block in Run.java), but fragile and inconsistent. I'd suggest aligning Run.java with the KameletMain.java pattern.
3. Triple-redundant configuration in Run.java
The MCP block uses three configuration mechanisms: writeSetting(), another writeSetting() for camel.mcp.enabled, and programmatic main.configure().mcp().withEnabled(true). The existing features (health, metrics, console) each use a single writeSetting() call in Run.java, with programmatic wiring handled in KameletMain.java. I'd suggest following the same pattern for consistency.
4. Wrong JSON-RPC error code for malformed JSON
In StreamableMcpJsonRpcEngine, when Jsoner.deserialize() receives malformed JSON, it silently returns an empty JsonObject (Jsoner catches DeserializationException and returns the default). The engine then returns error code -32600 ("Invalid Request"). Per JSON-RPC 2.0 spec (section 5.1), malformed JSON should return -32700 ("Parse error"). An error IS returned, just with the wrong code — wrapping the deserialization in a try-catch to distinguish parse errors from invalid requests would improve spec compliance.
Minor suggestions
- Hardcoded server version:
SERVER_VERSION = "1.0.0"doesn't reflect the actual Camel version (4.22.0-SNAPSHOT). Consider using a Maven-filtered version property for better MCP client diagnostics. - Null params in
handleToolsCall: Whenparamsis null,toolNameis null and passes through to the tool provider.JbangEmbeddedMcpRequestHandlerhandles this gracefully, but returning-32602("Invalid params") at the engine level would be more robust for futureToolProviderimplementations.
Overall this is solid work — the architecture, security annotations, and documentation are all well done. The main items to address are the host/port defaults (finding 1) and the listener overwrite inconsistency (finding 2). 👍
|
AI-generated comment on behalf of atiaomar1978-hub Pushed latest commit addressing @gnodet's review on PR #25203:
Tests: |
|
Can we slow down a bit and think things through before PR approval. From the Jira ticket:
Quarkus already has a solution for MCP optimized for the runtime. So some analysis is needed here and in Camel Quarkus for how best to leverage that. There's also a notes section in the ticket:
Not sure if any of those points have been addressed here? |
|
Hello @davsclaus @atiaomar1978-hub @jamesnetherton please hold before merging this PR, yesterday I opened https://issues.apache.org/jira/browse/CAMEL-24308 that will introduce new camel component that integrate ai-tools with an embedded MCP Server, it will provide a default implementation, and a specific implementation for quarkus (based on quarkus-mcp-server), the configuration of the MCP Server will be done via properties, like that bridges the ai-tools with the MCP. I am a little bit concerned about this PR, in particular the configuration part |
|
AI-generated comment on behalf of atiaomar1978-hub Fixed the sourcecheck failure in run 30559900193 ( The build regenerated Synced:
Re-ran MCP tests locally — green ( |
|
AI-generated reply on behalf of atiaomar1978-hub — no code changes in this update. Thanks @jamesnetherton — fair point, and I agree we should not rush this to merge without aligning on the broader design first. To answer your question directly: most of the JIRA “notes” questions are not fully resolved in this PR. This changeset is intentionally a narrow, dev-oriented slice:
So this PR is best seen as plumbing + JBang dev flag, not the full CAMEL-23853 vision from the ticket description. Happy to pause merge, open a short design discussion (possibly linked to @Croway’s CAMEL-24308), and clarify boundaries with Quarkus/Spring Boot before we call it done. Let me know whether you’d prefer this PR closed and redesigned, converted to draft pending CAMEL-24308 alignment, or scoped down further (e.g. JBang-only behind |
|
AI-generated reply on behalf of atiaomar1978-hub — no code changes in this update. Thanks @Croway — noted on CAMEL-24308. I share the concern about two configuration surfaces ( This PR was scoped as dev/management-only ( Proposal: hold merge on this PR until we align with CAMEL-24308 — either unify property names/prefixes up front, or clearly document that this path is JBang-local-dev-only and defer production embedded MCP to the component work you’re doing. Happy to join a design thread on CAMEL-24308 / CAMEL-23853 and adjust scope accordingly. No push from my side to merge before that. |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Prior review findings (host/port defaults, listener append, triple-redundant config, JSON-RPC error codes, server version) are all addressed — good improvement. The remaining concern is the design alignment with CAMEL-24308, which committers have already flagged.
Observations:
-
CAMEL-24308 namespace alignment — The
camel.mcp.*configuration namespace introduced by this PR will conflict with CAMEL-24308'scamel.server.mcp-*properties. Committers @Croway and @jamesnetherton have raised this concern, and the contributor agreed to hold merge pending alignment. This should be resolved before proceeding. -
Minor: JUnit assertions in test —
McpConfigurationPropertiesTestuses JUnit assertions (assertEquals,assertNull) while the other two new test files in this PR correctly use AssertJ. Per project conventions, new test code should use AssertJ.
Positive notes:
- The overall architecture is clean:
EmbeddedMcpRequestHandleras SPI incamel-main,StreamableMcpJsonRpcEngineas a reusable JSON-RPC 2.0 engine, andJbangEmbeddedMcpRequestHandlerbridging to the JBangToolRegistry. - The
insecure:devsecurity annotation oncamel.mcp.enabledfollows established patterns for dev-only management features. The MCP endpoint correctly sits on the management HTTP server. - The Origin header CSRF check provides appropriate browser-origin protection for a dev-only feature.
- Test coverage via
StreamableMcpJsonRpcEngineTest(9 tests covering error paths, CSRF, malformed JSON, etc.) is solid.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
|
AI-generated comment on behalf of atiaomar1978-hub Review follow-up (
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review after latest commit (bf7e363). The AssertJ migration and additional test coverage look good. Two items remain:
1. CAMEL-24308 namespace alignment (high — design hold)
The camel.mcp.* configuration namespace introduced here will conflict with CAMEL-24308's planned camel.server.mcp-* properties. Both @Croway and @jamesnetherton flagged this, and the contributor agreed to hold merge pending alignment. No code changes since the prior review address this — it requires a design decision from committers on whether to unify the namespaces or clearly scope this PR's properties as dev-only. Noting this for visibility since the PR remains open (not draft) despite the hold agreement.
2. Test resource leak in EmbeddedMcpHttpServerTest (low)
mcpEndpointDelegatesToHandler starts a CamelContext + Vert.x ManagementHttpServer but calls camelContext.stop() without a try-finally block. If any assertion or HTTP call fails before reaching stop, the server and context leak for the remainder of the test JVM. Since this test doesn't extend CamelTestSupport, there's no automatic lifecycle cleanup. Wrapping in try-finally would be a straightforward fix.
Positive notes:
- Latest commit successfully migrates tests to AssertJ and adds good coverage (fluent setters, default values, 415 content-type rejection, empty body handling, CLI default-to-false)
- Security model is solid:
@Metadata(security = "insecure:dev")on enable flags, management-server-only exposure, localhost binding, Origin header CSRF protection, Content-Type validation, proper JSON-RPC error codes
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
|
AI-generated comment on behalf of atiaomar1978-hub Review follow-up (
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review after latest commits. Both code-level items from the prior review are now addressed:
- ✅ Test resource leak fixed —
EmbeddedMcpHttpServerTest.mcpEndpointDelegatesToHandlernow wraps the test body in try-finally, ensuringcamelContext.stop()runs even when assertions fail (552be2036eb) - ✅ AssertJ migration complete —
McpConfigurationPropertiesTestfully migrated with expanded coverage (defaults, fluent setters, content-type rejection, empty body handling)
The implementation is well-structured with solid security: @Metadata(security = "insecure:dev") on enable flags, management-server-only exposure, localhost binding, CSRF protection, Content-Type validation, and proper JSON-RPC error codes.
Remaining: The CAMEL-24308 namespace alignment (camel.mcp.* vs camel.server.mcp-*) is the sole remaining concern, pending committer design decision.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
|
AI-generated comment on behalf of atiaomar1978-hub Review follow-up (
|
|
Now that CAMEL-24308/24310/24311 are merged, this PR can be simplified considerably while keeping its scope (dev/diagnostics tools on the management server — a valid, complementary use case to the
One enabler is needed on the camel-mcp-server side: Claude Code on behalf of Federico Mariani (@Croway) |
|
The enabler mentioned above is now up: #25329 (CAMEL-24353) makes the Claude Code on behalf of Federico Mariani (@Croway) |
fa49389 to
4ac36e7
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Claude Code on behalf of davsclaus
Review: CAMEL-23853 — Embed MCP server on camel run/dev and Camel Main
The reworked PR is much cleaner — good simplification using VertxMcpServerEngine directly instead of a custom JSON-RPC engine. The architecture (listener + service + schema mapper) is well-separated, security gating via insecure:dev is correct, and the test coverage is solid.
Three items to address:
1. camel-mcp-server must not be a compile dependency on camel-jbang-core (blocking)
camel-mcp-server is added as a compile (default scope) dependency. This means every camel run foo.yaml pulls in the MCP server, Vert.x MCP transport, and the MCP Java SDK — even when --mcp is not used.
Other optional features (camel-opentelemetry2, camel-jfr, camel-observability-services) are NOT compile dependencies of camel-jbang-core. They are downloaded on demand via dependencies.add("camel:...") in Run.java, which you already do correctly. The mainListenerClasses property is resolved by class name string at runtime, so JbangDevMcpMainListener does not need to be compiled against.
Make camel-mcp-server pluggable: remove it as a compile dependency from camel-jbang-core/pom.xml, and move the mcp package classes (JbangDevMcpMainListener, JbangDevMcpServer, ToolMcpSchemas) so they are only resolved when camel-mcp-server is on the classpath (downloaded on demand). The runtime download in Run.java already handles fetching the JAR when --mcp is used.
2. Upgrade guide entry is for a new feature, not migration (minor)
The upgrade guide (camel-4x-upgrade-guide-4_22.adoc) describes how to use --mcp. Per project conventions, the upgrade guide is for migration content only — changed defaults, removed options, breaking changes. New features belong in the command documentation pages, where the --mcp flag is already documented.
3. FQCN used inline in ToolMcpSchemasTest (minor)
ToolMcpSchemasTest imports ToolDescriptor at the top but then uses the fully qualified name inline: org.apache.camel.dsl.jbang.core.commands.ai.ToolDescriptor.tool(...). Per project conventions, use the simple name since the import is already present.
Positive notes:
- Clean architecture:
JbangDevMcpMainListener(lifecycle),JbangDevMcpServer(service with properdoStart/doStop),ToolMcpSchemas(schema mapping) - Correct
insecure:devsecurity gating on bothcamel.management.mcpEnabledandcamel.jbang.mcp - Management-server-only exposure with smart router type resolution fallback
- Good test coverage with proper try-finally cleanup and AssertJ assertions
- Clear distinction from business
camel.server.mcpEnableddocumented in the upgrade guide
This review does not replace specialized AI review tools or static analysis. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| <groupId>org.apache.camel</groupId> | ||
| <artifactId>camel-kamelet-main</artifactId> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
This should not be a compile dependency. Other optional features (camel-opentelemetry2, camel-jfr, etc.) are downloaded on demand via dependencies.add("camel:...") in Run.java — you already do this correctly for MCP. Having it as compile scope means every camel run foo.yaml pulls in the MCP server, Vert.x MCP transport, and MCP Java SDK even when --mcp is not used.
Remove this compile dependency and make the MCP classes pluggable — loaded at runtime only when camel-mcp-server is on the classpath (downloaded on demand when --mcp is passed).
There was a problem hiding this comment.
Fixed in 7f46187: removed camel-mcp-server compile dependency from camel-jbang-core. JBang dev MCP classes now live in camel-mcp-server/jbang and load on demand when Run.java downloads camel:mcp-server via --mcp.
AI-generated reply on behalf of atiaomar1978-hub
| main server via `camel.server.mcpEnabled`. | ||
|
|
||
| The `camel cmd route-diagram` and `camel cmd route-topology` now accept one or more Camel route source files | ||
| (instead of only the name/pid of a running integration), so diagrams and inter-route topology can be |
There was a problem hiding this comment.
The upgrade guide is for migration content only (changed defaults, removed options, breaking changes). This entry describes a new feature — it belongs in the command documentation pages where --mcp is already documented. Please remove this section from the upgrade guide.
There was a problem hiding this comment.
Fixed in 7f46187: removed the --mcp section from camel-4x-upgrade-guide-4_22.adoc. The feature remains documented on the command pages where --mcp is already described.
AI-generated reply on behalf of atiaomar1978-hub
| void buildsRequiredParametersSchema() { | ||
| String schema = ToolMcpSchemas.inputSchemaJson( | ||
| org.apache.camel.dsl.jbang.core.commands.ai.ToolDescriptor.tool("demo", "Demo tool") | ||
| .param("name", "string", "A name", true) |
There was a problem hiding this comment.
Per project import style conventions: ToolDescriptor is already imported at line 20 — use the simple name here instead of the FQCN.
| .param("name", "string", "A name", true) | |
| ToolDescriptor.tool("demo", "Demo tool") |
There was a problem hiding this comment.
Fixed in 7f46187: ToolMcpSchemasTest removed; schema building is inlined in JbangDevMcpServer (camel-mcp-server/jbang). Parameter schema coverage moved to JbangDevMcpServerTest using simple ToolDescriptor name per import conventions.
AI-generated reply on behalf of atiaomar1978-hub
|
Claude Code on behalf of davsclaus To clarify finding #1 from the review: the three new classes in These classes should be moved to |
Review feedback addressed (
|
| Review item | Fix |
|---|---|
Compile dependency on camel-mcp-server |
Removed from camel-jbang-core. Dev MCP classes moved to camel-mcp-server (org.apache.camel.component.mcp.server.jbang.*), loaded when Run.java downloads camel:mcp-server on --mcp. ToolRegistry accessed reflectively to avoid Maven reactor cycle. |
Upgrade guide --mcp section |
Removed from camel-4x-upgrade-guide-4_22.adoc (migration-only doc). |
FQCN in ToolMcpSchemasTest |
Test removed; schema coverage in JbangDevMcpServerTest with proper imports. |
Tests
mvn install -pl components/camel-ai/camel-mcp-server -DskipTests
mvn test -pl dsl/camel-jbang/camel-jbang-core -Dtest=RunMcpOptionTest,JbangDevMcpServerTest
4 tests passing.
Note: CAMEL-24308 namespace alignment (camel.mcp.* vs camel.server.mcp-*) remains pending committer design decision as previously agreed.
Grok review follow-up (
|
Croway
left a comment
Croway
left a comment
There was a problem hiding this comment.
Re-reviewed after 4ac36e7/00b8a87/7f46187/2d65a79 — the refactor looks right: the hand-rolled JSON-RPC engine, the core EmbeddedMcpRequestHandler SPI, and the camel.mcp.* namespace are gone, camel-jbang-core no longer compile-depends on MCP, and the latest commit already fixes tool-error messages and moves to ClassResolver. I also verified the non-obvious lifecycle concern: the HTTP servers are registered as early deferred services during autoconfigure, afterConfigure adds JbangDevMcpServer after them, and early-start preserves insertion order — so the routers are always in the registry before doStart() runs. 👍
Two findings worth addressing before merge (inline): the shared-port fallback silently serving the dev tools on the public main server, and camel.management.mcpEnabled being inert outside JBang. Minor items also inline.
Other small notes:
--mcpforcing the management host to127.0.0.1also rebinds health/metrics when combined with--observe/--health, which could break container readiness probes — worth a line in the--mcpoption description.- The
camel-mcp-server/pom.xmlchange is a pure reorder of two test dependencies — can be dropped to keep the diff minimal.
Claude Code on behalf of Federico Mariani (@Croway)
| return out; | ||
| } | ||
|
|
||
| private String resolveTargetServerType() { |
There was a problem hiding this comment.
This fallback makes the shared-port case work, but it silently defeats the 127.0.0.1 intent: both camel.server and camel.management default to port 8080, and ManagementHttpServer.doInit reuses the main VertxPlatformHttpServer when the ports match. In that case the withHost("127.0.0.1") set by KameletMain is ignored (the reused server was built from the main config, host 0.0.0.0), so camel run api.yaml --mcp — any app with a platform-http route — serves the ~40 introspection tools on 0.0.0.0:8080/mcp.
The dev console has the same trait, so this may be acceptable for an insecure:dev flag, but it should not be silent: please log a WARN here when no management-typed router is found and the MCP endpoint falls back to the main server, and mention the shared-port behavior in the --mcp option description.
| * Whether to expose dev/diagnostics MCP tools on this management server (requires camel-mcp-server on the | ||
| * classpath). Not intended for production use. | ||
| */ | ||
| public void setMcpEnabled(boolean mcpEnabled) { |
There was a problem hiding this comment.
This property is documented in main.adoc as a general camel.management.* option, but the only reader is JbangDevMcpMainListener, which is registered only when KameletMain injects it via mainListenerClasses for --mcp. A plain camel-main user who sets camel.management.mcpEnabled=true (with camel-mcp-server on the classpath) gets no endpoint and no error.
Contrast camel.server.mcp-enabled, which camel-main wires via the McpServerFactory SPI. Either state in the description that this option is currently only honored by Camel JBang (--mcp), or wire it in camel-main the same SPI way so it works (or fails meaningfully) everywhere.
There was a problem hiding this comment.
Fixed in cc569af (6f857dc): javadoc and main.adoc clarify camel.management.mcpEnabled is only honored when Camel JBang registers JbangDevMcpMainListener (camel run --mcp); route-based MCP uses camel.server.mcpEnabled on HttpServerConfigurationProperties.
AI-generated reply on behalf of atiaomar1978-hub
|
|
||
| @SuppressWarnings("unchecked") | ||
| private List<Object> allToolDescriptors() throws ReflectiveOperationException { | ||
| Class<?> registry = camelContext.getClassResolver().resolveClass(TOOL_REGISTRY); |
There was a problem hiding this comment.
ClassResolver.resolveClass returns null when the class is missing, so this NPEs on getMethod instead of failing with a clear error. resolveMandatoryClass throws ClassNotFoundException naming the class — much better startup diagnostics if someone enables camel.management.mcpEnabled without JBang on the classpath (same in createToolContext and executeTool).
There was a problem hiding this comment.
Fixed in cc569af: switched to resolveMandatoryClass via mandatoryClass() helper for ToolRegistry/ToolContext reflective access.
AI-generated reply on behalf of atiaomar1978-hub
| </dependency> | ||
| <dependency> | ||
| <groupId>com.networknt</groupId> | ||
| <artifactId>json-schema-validator</artifactId> |
There was a problem hiding this comment.
json-schema-validator is not used by either new test (leftover from the removed ToolMcpSchemasTest?) — can be dropped.
There was a problem hiding this comment.
Kept json-schema-validator test dependency (6f857dc clarifies comment): parent property resolves to 2.0.1 which provides Dialects required by MCP SDK; wiremock transitively pulls 1.5.x without it. Removing it breaks JbangDevMcpServerTest.
AI-generated reply on behalf of atiaomar1978-hub
| } | ||
|
|
||
| @Test | ||
| void buildsInputSchemaForParameterizedTools() { |
There was a problem hiding this comment.
Nit: this test exercises the ToolDescriptor builder (which lives elsewhere), not the schema JSON built in JbangDevMcpServer. Now that the HTTP test asserts inputSchema via listTools, consider strengthening that assertion instead (e.g. also check it contains "required" for the mandatory param) and dropping or renaming this one.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Review: CAMEL-23853 — Embed MCP server on camel run/dev and Camel Main
Thank you for this contribution — the overall architecture is sound: reflective bridge from camel-mcp-server to JBang's ToolRegistry, proper 127.0.0.1 binding default, correct security: "insecure:dev" annotation consistent with other dev features, and a nice integration test covering MCP protocol handshake, tool listing, schema validation, and tool execution.
A few issues to address before this can be merged:
1. Inconsistent mcpEnabled description across generated metadata (Medium)
The catalog copy (catalog/camel-catalog/src/generated/resources/.../camel-main-configuration-metadata.json) has a short description:
"Whether to expose dev/diagnostics MCP tools on this management server (requires camel-mcp-server on the classpath). Not intended for production use."
But the core copy (core/camel-main/src/generated/resources/META-INF/camel-main-configuration-metadata.json) and the AsciiDoc docs have the longer description mentioning JbangDevMcpMainListener and camel.server.mcpEnabled. These should be identical — please regenerate consistently.
2. Reference to non-existent camel.server.mcpEnabled (Medium)
See inline comment. The setter Javadoc references camel.server.mcpEnabled which does not exist in the codebase. If it is planned for a follow-up PR, the current description should not reference it yet.
3. Missing upgrade guide entry (Medium)
The PR description states the feature is documented in the 4.22 upgrade guide, but the diff contains no changes to any camel-4x-upgrade-guide-*.adoc file. New configuration properties (camel.management.mcpEnabled, camel.management.mcpPath, camel.jbang.mcp) and CLI flags (--mcp) should be noted in the upgrade guide.
4. {code ...} in generated docs (Low)
The Javadoc {@code JbangDevMcpMainListener} and {@code camel run --mcp} are rendered as literal {code ...} (without the @) in the generated AsciiDoc and JSON metadata. Consider using the @Metadata(description=...) annotation directly on the field with the full description text to avoid this.
Note: This review evaluates the PR against project conventions and standards. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| /** | ||
| * Whether to expose dev/diagnostics MCP tools on this management server (requires camel-mcp-server on the | ||
| * classpath). Currently honored only when Camel JBang registers {@code JbangDevMcpMainListener} (for example | ||
| * {@code camel run --mcp}); plain camel-main users should use {@code camel.server.mcpEnabled} for route-based MCP. |
There was a problem hiding this comment.
This Javadoc references camel.server.mcpEnabled for "route-based MCP", but that property does not exist anywhere in the codebase. This will be confusing for users who read this and attempt to use it.
If camel.server.mcpEnabled is planned for a follow-up, consider removing the reference until it ships. If it already exists elsewhere, please point me to it.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
There was a problem hiding this comment.
Addressed in 6f857dc: camel.server.mcpEnabled exists on HttpServerConfigurationProperties (see BaseMainSupport.setupMcpServer). Updated javadoc/docs to reference it explicitly instead of implying it is missing.
AI-generated reply on behalf of atiaomar1978-hub
| <version>${mcp-java-sdk-version}</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <!-- MCP SDK schema validation requires networknt 2.x; wiremock pulls 1.5.x without Dialects --> |
There was a problem hiding this comment.
The comment says "requires networknt 2.x" but ${networknt-json-schema-validator-version} resolves to 1.5.9 (a 1.x version). If 2.x is actually required by the MCP SDK, the version override is insufficient. If 1.5.x works fine, the comment is misleading — consider clarifying.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
There was a problem hiding this comment.
Clarified in 6f857dc: the comment now states 2.0.1 explicitly. ${networknt-json-schema-validator-version} in parent pom.xml is 2.0.1 (not 1.5.9); dependency tree confirms 2.0.1:test on camel-jbang-core. Wiremock brings 1.5.x transitively which lacks Dialects.
AI-generated reply on behalf of atiaomar1978-hub
|
in camel-kamelet-main you can add the classes you load dynamic to a known list of classes that maps to maven dependency then it will automatic download the needed JAR. |
|
Addressed remaining review feedback and automated review findings in commit
@Croway (replies on each thread below) Grok/Bugbot fixes
Tests: AI-generated comment on behalf of atiaomar1978-hub |
|
Fixed CI failure (uncommitted catalog generated files) in commit AI-generated comment on behalf of atiaomar1978-hub |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thank you for the contribution, @atiaomar1978-hub — this is a well-structured PR that follows existing patterns for similar JBang features (--console, --observe, --openapi-ui). CI is green, security model is correctly applied, and tests cover both CLI parsing and end-to-end integration.
Findings
1. [Question] Management server host override side effect
When --mcp is enabled, KameletMain unconditionally sets withHost("127.0.0.1") on the management server. This is good for security, but it has a side effect: if a user runs camel run app.yaml --mcp --observe, the management server (health, metrics, info) will also be bound to 127.0.0.1 instead of 0.0.0.0. This could break external health probes (e.g., Kubernetes liveness/readiness checks).
The CLI help text documents this, which is good. But is the intent that users who need both --mcp and externally-accessible health checks would configure camel.management.host separately? If so, does the property file value override the programmatic withHost() call?
2. [Minor] parameters() returns empty map in JbangDevMcpServer.toMcpTool()
The anonymous McpServerTool implementation returns Map.of() from parameters() while inputSchemaJson() is populated from the descriptor. This works because VertxMcpServerEngine.toolAdded() only uses inputSchemaJson(), but could be a gap if a future engine uses parameters() instead.
Positive observations
- Security model correctly applied:
security = "insecure:dev"onmcpEnabledin bothHttpManagementServerConfigurationPropertiesandCamelJBangConstants - 127.0.0.1 binding by default prevents remote access to dev MCP tools
- Fallback warning in
resolveTargetServerType()clearly notes security implication - Tests follow project conventions (package-private, AssertJ, no
Thread.sleep()) JbangDevMcpServerTestis a solid integration test covering server info, tool listing, schema structure, and tool execution- Generated files and documentation are all consistent with source changes
This review covers project rules and conventions. It does not replace specialized review tools (CodeRabbit, SonarCloud) or static analysis.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
| if (mcp) { | ||
| configure().httpManagementServer().withEnabled(true); | ||
| configure().httpManagementServer().withMcpEnabled(true); | ||
| configure().httpManagementServer().withHost("127.0.0.1"); |
There was a problem hiding this comment.
This unconditionally overrides the management server host to 127.0.0.1 when MCP is enabled. If a user also uses --observe for health/metrics, external probes (e.g. Kubernetes liveness checks) won't be able to reach the management endpoints.
The CLI help documents this side effect, which is good. But is there a way for the user to override this back (e.g. via camel.management.host in properties)? If property-file values are applied after this code, it would work — worth confirming the ordering.
There was a problem hiding this comment.
Fixed in cc4679e: withHost("127.0.0.1") is now guarded by !isConfigured("camel.management.host"), matching the pattern used elsewhere in KameletMain. Users combining --mcp with --observe can set camel.management.host explicitly for external probes.
AI-generated reply on behalf of atiaomar1978-hub
| @Override | ||
| public String name() { | ||
| return toolName; | ||
| } |
There was a problem hiding this comment.
Minor: parameters() returns Map.of() while inputSchemaJson() is populated from the descriptor's params. This works today because VertxMcpServerEngine.toolAdded() only uses inputSchemaJson(), but could be a gap for forward compatibility if a future engine implementation relies on parameters() instead.
There was a problem hiding this comment.
Fixed in cc4679e: descriptor params are converted to a flat metadata map and parsed with AiToolParameterHelper.parseParameterMetadata(). Both parameters() and inputSchemaJson() now share the same ParameterDef map via buildJsonSchemaFromDefs().
AI-generated reply on behalf of atiaomar1978-hub
|
Addressed the latest review feedback in cc4679e:
Tests run: AI-generated comment on behalf of atiaomar1978-hub |
Rebased onto main and refactored per review feedback (Croway, CAMEL-24308): - Drop hand-rolled StreamableMcpJsonRpcEngine and camel.mcp.* namespace - Reuse VertxMcpServerEngine on the management HTTP router (CAMEL-24353) - Expose JBang ToolRegistry tools via JbangDevMcpServer + --mcp flag - Configure via camel.management.mcpEnabled/mcpPath and camel.jbang.mcp - Add RunMcpOptionTest, JbangDevMcpServerTest, ToolMcpSchemasTest Co-authored-by: Cursor Agent <noreply@cursor.com>
When the management server reuses the main HTTP server on the same port, only a server-typed VertxPlatformHttpRouter exists. Resolve the MCP engine target dynamically so --mcp works with --port and co-hosted setups. Co-authored-by: Cursor Agent <noreply@cursor.com>
Remove camel-mcp-server compile dependency from camel-jbang-core. Move JBang dev MCP classes to camel-mcp-server/jbang and resolve ToolRegistry reflectively to avoid Maven reactor cycles. Update KameletMain listener class name, remove --mcp upgrade guide section, and keep integration tests in jbang-core with test-scoped mcp-server. Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
Resolve JBang tool classes via CamelContext ClassResolver instead of Class.forName, unwrap invocation failures for MCP error responses, and assert inputSchema is published for parameterized tools in tests. Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
- Log a warning when MCP falls back to the main HTTP server router - Clarify camel.management.mcpEnabled is only honored by camel run --mcp - Use resolveMandatoryClass for reflective ToolRegistry access - Update --mcp option description for shared-port bind address behavior - Remove redundant ToolDescriptor builder unit test; HTTP test covers schemas - Keep json-schema-validator 2.x test dep (MCP SDK needs it; wiremock pulls 1.5.x) Co-authored-by: Cursor <cursoragent@cursor.com>
- Clarify camel.server.mcpEnabled exists (HttpServerConfigurationProperties) - Fix networknt pom comment to reference actual 2.0.1 version - Download MCP deps when profile camel.jbang.mcp=true (not only --mcp CLI) - Register JbangDevMcpMainListener in known-dependencies for auto-download - Strengthen inputSchema test with required field assertion - Document 127.0.0.1 management rebind when --mcp is used with --observe Co-authored-by: Cursor <cursoragent@cursor.com>
…led docs Sync camel-catalog generated main.adoc and configuration metadata with core/camel-main after mcpEnabled javadoc update (fixes CI uncommitted changes check). Co-authored-by: Cursor <cursoragent@cursor.com>
- Only bind management host to 127.0.0.1 when camel.management.host is unset
- Populate JbangDevMcpServer.parameters() via AiToolParameterHelper
- Fix mcpEnabled docs: plain-text javadoc avoids {code ...} in generated metadata
- Regenerate catalog/main configuration metadata
Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
Rebased onto upstream/main and regenerated jbang/catalog metadata to merge main changes with --mcp command options. Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
cc4679e to
c7a7be1
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thank you for this contribution, @atiaomar1978-hub — this is well-structured work that follows established JBang patterns (--console, --observe, --openapi-ui). The architecture is clean, the security model is correctly applied, and the code has been thoroughly refined across multiple review rounds.
Summary
- Security model correctly applied:
security = "insecure:dev"on bothcamel.jbang.mcpandcamel.management.mcpEnabled, management-server-only exposure,127.0.0.1binding by default - Reflective bridge to
ToolRegistryis architecturally sound — avoids compile dependency from component to CLI module JbangDevMcpServerTestis a solid integration test covering MCP handshake, tool listing, schema validation, and tool execution- All prior review findings addressed
Minor notes (non-blocking)
- CI — needs to be triggered and pass (first-time contributor approval required)
json-schema-validatorversion override — the 1.5.x → 2.0.1 bump in test scope is explained in the comment; CI will validate no WireMock test breakage--mcp+--observeinteraction — management host forced to127.0.0.1could affect container health probes; documented in CLI help text, andcamel.management.hostoverrides it
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
|
rebase this on main |
Sync catalog jbang metadata with camelWrapper from main and keep mcp entry from this PR. Regenerate camel-spring.xsd with allowedSchemes. Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
6 participants
Summary
camel.mcp.*configuration and an MCP Streamable HTTP endpoint on the embedded management HTTP server (/mcpby default), wired throughEmbeddedMcpRequestHandler.camel run/camel dev--mcp(andcamel.jbang.mcp) to enable embedded MCP with JBangToolRegistrytools against the local process (management server bound to127.0.0.1by default).Test plan
mvn test -pl components/camel-platform-http-main,dsl/camel-jbang/camel-jbang-core -am -Dtest=StreamableMcpJsonRpcEngineTest,EmbeddedMcpHttpServerTest,RunMcpOptionTestcamel run hello.yaml --mcpthen POST JSON-RPCtools/listtohttp://127.0.0.1:8080/mcphttps://issues.apache.org/jira/browse/CAMEL-23853
AI-generated PR description on behalf of atiaomar1978-hub
Made with Cursor