Repository navigation
Conversation
…re-limit, and failed-call retry budget Three bugs fixed in camel-jbang-mcp: 1. AuthoringTools.call() created a new ToolContext() on every invocation, so the CamelCatalog was rebuilt from scratch on each tool call. Fix: inject CatalogService and pre-populate the ToolContext with its shared catalog before executing the registry tool. Added ToolContext.setCatalog() to allow injecting a pre-built catalog instance. 2. KameletTools.camel_catalog_kamelets() called KameletCatalogHelper.loadKamelets() on every request (no caching) and applied the limit before sorting, so results depended on HashMap iteration order and were non-deterministic across calls. Fix: added a ConcurrentHashMap cache keyed by version; moved sorting before the limit so the first N alphabetical kamelets are always returned. Added a 'total' field to KameletListResult to expose how many matched before limiting. 3. RepeatedToolCalls.repeatOf() incremented the call count unconditionally, so a call that threw an exception consumed a slot in the FULL_ANSWERS budget and a legitimately failed call could prevent the model from getting a full answer on retry. Fix: split counting into repeatOf() (peek-only) and a new recordSuccessfulAnswer() method called by the caller only after the tool returns without throwing. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
🔄 Backport BotThis bugfix targets
Add a Backport PRs will be created automatically on merge (once a port label is added). Comment ℹ️ If you push additional commits after |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 5 of 703 tested, 8 compile-only — current: 5 all testedMaveniverse Scalpel detected 5 affected modules (current approach: 5). Skip-tests mode would test 5 modules (4 direct + 3 downstream), skip tests for 8 (generated code, meta-modules) Modules Scalpel would test (5)
Modules with tests skipped (8)
💡 Manual integration tests recommended:
All tested modules (15 modules, 3m 14s total)Total reactor time: 3m 14s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @gnodet for picking up CAMEL-25640 and working through the three parts of it, with tests for each. The Kamelet sort-before-limit fix and the total field are good, and so is the clearer "Failed to load kamelet" message.
I found one regression and two places where the fix does not do what the description says. Details below, several with inline comments.
New findings:
-
Regression:
camel_error_diagnoseno longer gets the repeat note.repeatOf()now only peeks, and the count only grows throughrecordSuccessfulAnswer().DiagnoseTools.camel_error_diagnose(DiagnoseTools.java:69-75, not in this diff) still calls onlyrepeatedCalls.repeatOf(...), so its count stays at 0 and the third, tenth, hundredth identical diagnose is answered in full. That is one of the four tools the "Repeated calls" section ofcamel-jbang-mcp.adocnames. It needs the matching call after a successful diagnose, for example:JsonObject answer = ErrorDiagnoser.diagnose(error, catalog); if (repeatedCalls != null) { repeatedCalls.recordSuccessfulAnswer(connection, "camel_error_diagnose", args); } return answer;
(with the args map built once and reused for both calls). A test in
DiagnoseToolsTestthat the third identical call isrepeatedwould have caught this. -
The shared catalog is dropped as soon as
camelVersionis given (AuthoringTools.java:403-412). All four deterministic registry tools start withapplyVersion(ctx, args), which callsctx.setCamelVersion(version). On a freshToolContextthe version is null, so it differs andsetCamelVersionsetscatalog = null. The tool then lazy-loads throughCatalogLoader.loadCatalog(null, version, true), with no cache and withoutcamel.catalog.repos, the same as before this PR. The expensive case from the JIRA (a downloaded catalog plus a new classloader on every call) is therefore not fixed. Only the no-version case gets the caching catalog. Worse, a version that cannot be downloaded is now tried twice per call: once inCatalogService(which does not cache the failure) and once inToolContext. Setting the version on the context before the catalog (or asetCatalog(version, catalog)) would keep it. Note thatCatalogServicethen answers from the default catalog when the download fails, where the tool used to fail, so that is a behaviour choice to make on purpose. -
The Camel TUI MCP server still counts failed calls (
TuiMcpServer.java:541-542). For the shared tools,TuiToolRegistry.executeSharedcatchesToolExecutionExceptionand returns"Error: ..."as normal text, soexecute()does not throw andrecordSuccessfulAnswerruns for a failedcamel_catalog_docwith a badcamelVersion, or acamel_validate_sourcewithoutfile. The second commit is still needed (without it the TUI would never give the note), but the "a failed call does not count" part of the fix does not hold on this path. -
Question on the design of point 3 of the JIRA. CAMEL-25075 counted before running on purpose: the note is there to break a model that is stuck. Many failures are deterministic too: a missing
file, an unsupported file extension, an unknown version. The model now gets the same error on every attempt and never the note, which is the loop CAMEL-25075 was made for. Only transient failures (a catalog download that times out) gain from a retry. Would it be better to exempt only those, or to give failed calls a few extra full answers instead of an unlimited number? Either way, the "Repeated calls" section ofdocs/user-manual/modules/ROOT/pages/camel-jbang-mcp.adocshould say that a failed call does not count. -
CatalogServiceTest.quarkusRuntimeWithCamelVersionResolvesTheBomFromRegistry(CatalogServiceTest.java:192) does not reach the new code.BUILTIN_VERSIONmatchessameAsDefault(), sohasVersionbecomes false andloadCatalogtakes the older no-version Quarkus path. It is also disabled in CI, so theCatalogServicechange is untested. A released version that is not the built-in one (e.g.4.18.0), with an assertion on the version of the returned catalog, would test the fix. -
Non-blocking:
CatalogService.java:229-230says "we guard with an assertion", but there is no assertion. That branch looks unreachable now (every Quarkus path reachesdoLoadCatalogwith a BOM or a version). Please drop the comment, or replace thereturnwith an explicitIllegalStateException. The new branch at:213-218calls.quarkusCamelBom()on the result offindQuarkusPlatformBom, which returns null when the registry cannot be read. That gives an NPE instead of a readable message. The no-version path above has the same issue. -
Non-blocking:
Math.max(1, limit)inKameletTools.java:76andCatalogTools.java:56turnslimit=0or a negative limit into one result. The JIRA suggests the default, asExampleToolsdoes (limit > 0 ? limit : 50). TheCatalogToolschange is also missing from the PR description. -
Non-blocking:
KameletToolsTestdownloadscamel-kamelets-catalogfrom Maven Central at test time (the module has no dependency on it), and it is not disabled in CI like the other download tests in this module.secondCallIsServedFromCachealso cannot fail when the cache is missing, because it only compares two results. A package-private loader hook (or a counter) would make it a real cache test and keep it offline. -
Non-blocking:
call()reads aruntimeargument (AuthoringTools.java:405), but none of the four tools that go throughcallDeterministichas one, so the code never runs. If a tool with aruntimeargument such ascamel_dependency_for_classis ever routed here, it would quietly switch it to a Spring Boot or Quarkus catalog. I would leaveruntimeout. The other registry tools (camel_project_overviewwithcamelVersion,camel_dependency_for_class, ...) still build a catalog per call throughcall(tool, args). That is fine for this PR, but the JIRA reads as covering all of them. -
Scope: the JIRA also lists
LAST_REFUSED(a static, unbounded map shared across MCP connections) and the deterministic tools that are not marked yet. Neither is in this PR, so CAMEL-25640 should stay open or get a follow-up issue when this is merged. The second commit (fix: record successful TUI ...) has noCAMEL-25640:prefix. The squash merge takes care of that. The backport bot suggestscamel-4.22.x, butRepeatedToolCallsdoes not exist on that branch (CAMEL-25075 shipped in 4.23.0), so only the Kamelet andCatalogServiceparts could go there.
Checked against the project conventions in CLAUDE.md. This is a rules-and-conventions review and does not replace CodeRabbit, Sourcery or SonarCloud.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…sion ordering, TUI error counting, and non-blocking suggestions
findQuarkusPlatformBom() returns null when the registry cannot be reached or no platform matches the requested Camel version. Both call sites in CatalogService (no-version Quarkus path and version-given Quarkus path) now check for null and throw a ToolCallException with a readable message instead of propagating a NullPointerException.
|
Thank you for the detailed review, @davsclaus! All findings have been addressed:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Second look on the CAMEL-25640 fix. All three blocking findings from the first review are addressed:
✅ Finding 1 — camel_error_diagnose missing recordSuccessfulAnswer: Fixed in DiagnoseTools.java (lines 78-81). The args map is now built once and reused for both repeatOf and recordSuccessfulAnswer. New test thirdIdenticalCallIsRepeated provides the missing regression coverage.
✅ Finding 2 — Shared catalog dropped on setCamelVersion: Fixed in AuthoringTools.call(tool, args, catalogService). The version is set on the ToolContext before setCatalog, so the assignment is a no-op (same value) and the pre-loaded catalog survives.
✅ Finding 5 — CatalogServiceTest tests the wrong code path: Fixed — the test now uses "4.10.0" instead of BUILTIN_VERSION, so sameAsDefault() is false and the new Quarkus registry lookup path is exercised.
✅ Finding 6 — No null guard after findQuarkusPlatformBom: Fixed. Both call sites now check for null and throw ToolCallException with a readable message. A defensive IllegalStateException guard is also in place at the bottom of doLoadCatalog for the BOM-already-resolved path.
✅ Finding 7 — Math.max(1, limit) uses wrong default: Fixed in both KameletTools and CatalogTools — now limit != null && limit > 0 ? limit : 50, matching ExampleTools.
One minor remaining nit on the TUI path:
The !text.startsWith("Error:") heuristic works for the ToolExecutionException path (the executeShared method always prefixes those with "Error: "), but it also matches any tool whose successful response starts with "Error:" — unlikely in practice, but fragile. The toolRegistry.execute() contract doesn't forbid it. A cleaner alternative would be to let executeShared return a typed result or throw on failure, but that's a broader refactor. As a lower-friction option: use a boolean sentinel or catch the exception higher up (since toolRegistry.execute() throws IllegalArgumentException which is already caught). Non-blocking for this PR — the current implementation is better than before.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…g from Maven Central)
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Both findings from the prior review were verified against the current diff.
⚠️ Finding 1 (medium) — Swallowed exception in AuthoringTools.call() with no diagnostic trace
The new catch (Exception e) block at line 413 silently discards failures from CatalogService.loadCatalog(). The fallback to ToolContext's own lazy-load is correct — it will surface its own error if it also fails — but the first failure is completely invisible. On a slow or flaky network this doubles the download latency with zero diagnostic trace, and the comment alone is not a substitute for a log statement.
Suggestion: add a LOG.debug(...) (or TRACE) call so the first failure leaves a footprint:
💡 Finding 2 (low) — startsWith("Error:") for success detection in TuiMcpServer is fragile
The if (!text.startsWith("Error:")) check on line 544 couples success detection to a string convention spread across executeShared() (which catches ToolExecutionException and returns "Error: " + msg) and 30+ hardcoded return "Error: ..." statements in TUI tool handlers. The coupling is real: any format change silently breaks the retry-budget accounting.
The practical risk today is low (all producers are in the same file and the convention is locally consistent), but a typed result — success/failure wrapper, or a checked exception that propagates out rather than being stringified — would make this robust. The verifier lowered severity from medium to low given the current scope.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…olContext lazy-load
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the two previously raised nits are still the only open observations; no new issues found in the latest commits.
Prior finding (PRRT_kwDOAAMl7c6sf6fO) — TUI !text.startsWith("Error:") heuristic: still open and unchanged. The heuristic works correctly for the ToolExecutionException path (where executeShared always returns "Error: ..."), but any tool whose successful response happens to start with "Error:" would incorrectly suppress recordSuccessfulAnswer. Non-blocking as noted previously — the implementation is still better than counting failed calls.
New observation (nit): KameletTools.kameletCache is an unbounded ConcurrentHashMap keyed by the user-supplied kameletsVersion string. In practice the number of distinct versions queried in a single server lifetime is tiny, and CatalogService.cache uses the same unbounded pattern, so this is consistent. Noting for awareness — if memory ever becomes a concern, a bounded LRU (like RepeatedCallSessions.sessions) would cap it.
State machine (RepeatedToolCalls.repeatOf + recordSuccessfulAnswer): the two increment paths are mutually exclusive — the pre-quota path goes through recordSuccessfulAnswer, the post-quota path through repeatOf's own merge(). No double-write. ✅
Pre-fix test validation:
aFailedCallDoesNotCountAgainstTheRetryBudget— would fail without the fix: the oldrepeatOfincremented on every call, so the third call (count=2) would return a note instead of running; the test'sassertThat(...).isNull()on the third call catches that. ✅thirdIdenticalCallIsRepeatedinDiagnoseToolsTest— would fail withoutrecordSuccessfulAnswerinDiagnoseTools: count would never increment (peek-only), third call returns null instead of the note. ✅
LGTM on the new commits.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
All prior findings are addressed. LGTM.
Previous findings resolved:
- ✅ Finding 1 (medium) —
CatalogService.loadCatalog()catch block now logs viaLOG.debugf(...)before falling back toToolContextlazy-load. - ✅ Finding 2 (low) —
startsWith("Error:")nit acknowledged; current implementation is correct for theToolExecutionExceptionpath and an improvement over counting failed calls.
New commits verified — no new issues found. The three bugs (catalog caching, kamelet sort-before-limit, failed-call retry budget) are correctly fixed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
3 participants
Summary
Three confirmed bugs in
camel-jbang-mcpon main, all introduced together with the MCP server feature.Root Causes & Fixes
1.
AuthoringTools.call()— CamelCatalog rebuilt on every tool callcall()created anew ToolContext()on every invocation.ToolContextholds theCamelCatalogand loads it lazily on first access — so every call triggered a fresh catalog load instead of reusing the shared instance.Fix: Inject
CatalogServiceintoAuthoringToolsand pre-populate theToolContextwith its cached catalog before executing the registry tool. AddedToolContext.setCatalog()to allow injecting a pre-built catalog instance.2.
KameletTools.camel_catalog_kamelets()— no caching, limit before sortKameletCatalogHelper.loadKamelets()was called on every request, rebuilding the downloader and classloader each time.limitguard was applied inside theHashMapiteration loop — before thesort— so results depended on hash iteration order and were non-deterministic across calls.Fix:
ConcurrentHashMap<String, Map<String, Object>> kameletCachekeyed by version. A privateloadKamelets(version)helper checks the cache first, usingputIfAbsentto handle concurrent first calls correctly.alllist, sort alphabetically, thensubList(0, maxResults).totaltoKameletListResultto expose how many kamelets matched before the limit was applied.camel_catalog_kamelet_docalso reuses the cache.3.
RepeatedToolCalls.repeatOf()— failed calls consume the retry budgetrepeatOf()incremented the call count unconditionally viacounts.merge(...)before the tool ran. A tool call that threw an exception consumed a slot in theFULL_ANSWERSbudget, so a model that legitimately retried a failed call could hit the repeat-note wall without ever getting a successful answer.Fix: Split counting into two phases:
repeatOf()now only peeks (getOrDefault) without incrementing. Returnsnullif count <FULL_ANSWERS.recordSuccessfulAnswer(tool, args)method — called by the caller only after the tool returns without throwing — increments the count.RepeatedCallSessionsgot a matchingrecordSuccessfulAnswer(McpConnection, tool, args)method.AuthoringTools.callDeterministic()callsrepeatedCalls.recordSuccessfulAnswer()after a successfulcall().Tests
RepeatedToolCallsTest— updated existing tests to use the new two-phase API; addedaFailedCallDoesNotCountAgainstTheRetryBudget().KameletToolsTest(new) — covers: repeated calls return identical sorted results, limit applied after sort,totalreflects untruncated count, type/name/description filters, kamelet doc detail, unknown kamelet error, cache hit on second call.CatalogServiceTest— added Quarkus+camelVersion regression test (network-disabled in CI).All 46 tests pass locally.
Hermes Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet