CAMEL-23873: Introduce shared ToolRegistry for AI tools#24337
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
💡 Manual integration tests recommended:
All tested modules (7 modules)
|
davsclaus
left a comment
There was a problem hiding this comment.
Review: Shared ToolRegistry, DevConsole tools, analysis tools, and diagnostic prompts
This review evaluates the PR against project rules and conventions. It does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analysis (SonarCloud).
Overall
Well-structured refactoring. The ToolRegistry pattern is clean, the registerStatusTool/registerRouteControlTool helpers eliminate boilerplate nicely, and backward compatibility is preserved (existing AskCliToolsTest tests pass, AskTools public API unchanged). The MCP prompts (camel_diagnose_route, camel_optimize_route) provide clear step-by-step workflows.
Findings
1. PR description overstates scope — MCP deduplication is not achieved yet (Medium)
The description claims this PR will "eliminate tool duplication between the MCP server and Agent REPL" and create a "single source of truth." However, the MCP server tools (RuntimeTools.java, CatalogTools.java, etc. in camel-jbang-mcp) are not refactored to use the new ToolRegistry — they retain their own Quarkus @Tool-annotated implementations backed by RuntimeService. Currently, ToolRegistry is consumed by AskTools only. This isn't a code defect, but the PR description should accurately reflect what was done vs. what's planned as follow-up.
2. Missing JIRA reference in commits (Low)
Per project conventions, commit format should be CAMEL-XXXX: Brief description. All 5 commits lack a JIRA reference.
3. sql_query tool should be marked destructive (Medium) — see inline comment
4. ToolContext.catalog() lazy init is not thread-safe (Low) — see inline comment
Questions
- Is refactoring the MCP server (
RuntimeTools,CatalogTools, etc.) to consumeToolRegistryplanned as a follow-up? - The 5th commit says "Remove planning document now that implementation is complete" but no file removal appears in the diff. Was this a file added and removed within this branch?
- The DevConsole tools (
get_startup_steps,get_datasources,get_spans,get_metrics) call actions/status sections that may not exist in all Camel applications. Has the behavior been verified for absent sections — does the IPC protocol return a clean "not available" response vs. an error? - The PR body has unchecked manual test items ("Manual verification with a running Camel application" and "MCP server smoke test"). These should ideally be verified before merge.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
davsclaus
left a comment
There was a problem hiding this comment.
Updated Review — PR #24337
Following up on my earlier review. The two new commits address all prior findings.
Prior findings — all addressed
sql_querydestructive flag — Now.readOnly(false).destructive(true). ✅ToolContext.catalog()thread safety — Proper double-checked locking withvolatilefields. ✅- MCP deduplication —
RuntimeToolsnow delegates toToolRegistryviadelegateToRegistry(), with onlycamel_runtime_processesandcamel_runtime_receiveremaining as MCP-specific. The "single source of truth" claim is now accurate. ✅
New changes look good
get_memoryenriched in ToolRegistry to combine memory/gc/threads sections — matches prior MCP behavior.get_route_topologynow has configurablemetric/externalparams instead of hardcoded values.trace_controlthrows on unknown action instead of silently passing through.toJsonObjecthelper handles the String→JsonObject conversion cleanly with a reasonable fallback.camel_runtime_receiveproperly documented as MCP-only.
Remaining (low severity, already flagged)
- Commit messages lack JIRA references per project conventions (
CAMEL-XXXX: ...format).
No blocking issues. The PR delivers on its stated goals — nice work consolidating the tool definitions.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
ebc70d8 to
550620f
Compare
davsclaus
left a comment
There was a problem hiding this comment.
Good refactoring — the shared ToolRegistry cleanly eliminates duplication between AskTools (Agent REPL) and RuntimeTools (MCP server), with each caller handling protocol-specific concerns and delegating execution to the shared registry.
A couple of things to be aware of:
-
Merge conflict with recent tools on
main: Since this PR was branched,maingainedcamel_runtime_heap_histogram(PR #24364, CAMEL-23870) andcamel_runtime_jfr_old_objects(CAMEL-23872) inRuntimeTools.java. These will need to be added toToolRegistrywhen resolving the merge conflict. -
The new MCP prompts (
camel_diagnose_route,camel_optimize_route) are well-structured guided workflows.
See inline comment for one behavioral change to verify.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Add a shared ToolRegistry in camel-jbang-core that serves as the single source of truth for AI tool definitions and execution logic, eliminating duplication between the MCP server and Agent REPL. New classes: - ToolRegistry: central registry of 50 shared tool descriptors with ToolDescriptor, ToolContext, and ToolExecutor functional interface - ToolDescriptor: builder-style descriptor bundling metadata + executor - ToolContext: shared execution context (RuntimeHelper + CamelCatalog) - ToolExecutionException: unchecked exception for tool failures Refactored consumers: - AskTools (Agent REPL): delegates to ToolRegistry for shared tools, keeps CLI/file tools locally (995 -> 478 lines) - RuntimeTools (MCP server): thin MCP wrappers delegating to ToolRegistry via delegateToRegistry() helper. Only camel_runtime_processes and camel_runtime_receive keep their own implementations. New tools added to ToolRegistry: - DevConsole: circuit breakers, startup steps, datasources, spans, metrics - Analysis: route analysis with anti-pattern hints, EIP stats, config drift detection - Catalog: dataformats, languages, EIP doc (previously MCP-only) New MCP prompts: - camel_diagnose_route: 7-step guided diagnosis workflow - camel_optimize_route: 6-step guided optimization workflow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
550620f to
4d6add7
Compare
|
Claude Code on behalf of Guillaume Nodet Thanks for the review! Both points addressed:
|
|
LGTM |
oscerd
left a comment
There was a problem hiding this comment.
Thanks for this — the ToolRegistry consolidation itself is clean: it's populated once in a static{} block and exposed only through unmodifiable views, ToolContext is per-consumer (no global mutable state, so multi-context isolation holds), and it's well covered by ToolRegistryTest. No public-API break. 👍
One blocking issue before merge, though: the two new MCP prompts reference tool names that don't exist on the MCP surface.
Tool-name mismatch in camel_diagnose_route / camel_optimize_route (PromptDefinitions.java)
The MCP server exposes the runtime tools under their method names in RuntimeTools.java — camel_runtime_context, camel_runtime_routes, camel_runtime_health, camel_runtime_top, etc. The two new prompts instead instruct the model to call get_context, get_routes, get_health, get_top_processors, … — the ToolRegistry / AskTools (REPL) names, which are not registered as MCP @Tools.
Two consequences:
- The ~13 tools that do have an MCP equivalent are systematically misnamed — e.g.
get_context→ should becamel_runtime_context,get_top_processors→camel_runtime_top,get_route_dump→camel_runtime_route_dump. - Six referenced tools have no MCP counterpart at all:
get_circuit_breakers,get_datasources,get_spans,get_metrics,get_route_analysis,get_eip_stats. These exist only inToolRegistryfor the REPL agent and are never surfaced as@Tool, so the "Advanced diagnostics" / infrastructure steps can't run when the prompt is used through the MCP server.
For reference, the existing sibling prompts in the same file (camel_build_integration, camel_migrate_project, camel_security_review) all use the fully-qualified camel_* names, so the two new prompts are the odd ones out.
Suggested fix: rename the ~13 to their camel_runtime_* equivalents, and for the 6 registry-only tools either expose them as MCP @Tools or drop those steps from the prompt text. A small test asserting that every tool name referenced in a @Prompt resolves to a registered MCP tool would prevent this class of drift (CI can't catch it today — the prompt bodies are just strings).
Minor (non-blocking)
- Delegation changes MCP output/error shapes: via
RuntimeTools.delegateToRegistry/toJsonObject, a "no status" case now returns{"result":"No status available…"}instead of throwing, and empty errors/history return{"result":…}instead of{"errors":[]}. Worth a note if any client relied on the old shapes. camel_runtime_route_topology:metric/externalnow default totruewhen omitted (wasfalse). It's mentioned in the PR description but not incamel-jbang-mcp.adoc.AskTools.setTargetPid(-1)is guarded byif (targetPid >= 0), so resetting to a negative pid leavesctx.pidpointing at the previously targeted process.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Summary
Claude Code on behalf of Guillaume Nodet
Introduce a shared ToolRegistry in
camel-jbang-corethat serves as the single source of truth for AI tool definitions and execution logic, eliminating duplication between the MCP server and Agent REPL.JIRA: CAMEL-23873
What changed
camel-jbang-core): Central registry of 51 shared tool descriptors withToolDescriptor(builder-style metadata + executor),ToolContext(shared execution context wrappingRuntimeHelper+CamelCatalog), andToolExecutorfunctional interface.delegateToRegistry(). Onlycamel_runtime_processesandcamel_runtime_receivekeep their own implementations.camel_diagnose_route(7-step diagnosis workflow),camel_optimize_route(6-step optimization workflow).camel_runtime_heap_histogram(CAMEL-23870) now delegates to ToolRegistry'sget_heap_histogram.Architecture
Behavioral note
The
metricandexternalparameters incamel_runtime_route_topologynow default totruewhen omitted (matching ToolRegistry defaults and prior MCP behavior). Previously the delegation refactor was silently flipping them tofalsewhen null.Test plan
ToolRegistryTest— 16 tests covering registration, execution, and edge casesdelegateToRegistry()correctly bridgesToolExecutionException→ToolCallException🤖 Generated with Claude Code