ramu11 commented on PR #29722:
URL: https://github.com/apache/camel/pull/29722#issuecomment-6847605291
> Thanks @ramu11, this round closes the blocking point from my last review.
Each variable now matches only RFC 3986 `pchar` or `%HH`
(`AiResourceUriTemplate.java:44`), `.` and `..` are rejected, and neither the
executor nor the bridge puts the concrete client URI in a log line or an error
message any more. So the CR/LF log forging, the `?`/`#` capture and the dot
segments from @Croway's reproducers are all gone, and
`matchesOnlyRfc3986PcharSegmentsAndRejectsUnsafeOrMalformedSegments` covers
them. The rest of my list is done as well:
>
> * overlapping templates now fail route startup in
`AiResourceRegistry.putTemplate` (line 160), and the snapshot is a `TreeMap`,
so the result no longer depends on JVM iteration order.
> * `{id}`/`{ID}` are rejected case-insensitively.
> * `{`/`}` in a fixed `resourceUri` fail startup.
> * the new headers have `@Metadata` and `headersClass`.
> * the `mcp-server.adoc` and `design/aiResource.adoc` updates are in, and
stdio now passes `title` through.
>
> All five gnodet-bot threads are fixed in the code.
>
> A few small things left:
>
> 1. **The fixed-resource error path lost its diagnostic**
(`McpServerBridge.java:573`). The line is now `LOG.warn("MCP resource '{}' read
failed", spec.getName())`, without `error.message()` and `error.cause()`. For a
route exception that is fine, because `AiResourceExecutor` already logs it at
ERROR. But "produced no content" and the conversion errors
(`AiResourceExecutor.java:95`, `:104-120`) are not logged anywhere else. A
route author who gets "Resource read failed" on a fixed resource, which also
worked like this before this PR, can no longer see why. Since the message now
holds only the resource name, we can add it back safely:
> ```java
> LOG.warn("MCP resource '{}' read failed: {}", spec.getName(),
error.message());
> ```
> 2. **FQCNs in the new test code** (CLAUDE.md "Imports" rule; CI does not
catch these): `McpStdioHarnessMain.java:84` (`org.apache.camel.CamelContext`),
`StdioMcpServerEngineTest.java:220` (`McpResourceReadHandler`), `:249`
(`McpResourceTemplateReadHandler`), `:304`
(`AiToolParameterHelper.ParameterDef`, on a line this PR already edits for
`Map`) and `StdioMcpServerProcessIT.java:234` (`java.util.function.Consumer`).
Please import them.
> 3. **Upgrade guide for the brace check**
(`AiResourceConsumer.java:111-115`): `ai-resource` shipped in 4.23.0. A 4.23
route with `{...}` in `resourceUri` used to start, and the SDK served it as a
pattern. With this PR it fails startup. Could you add a short entry to
`camel-4x-upgrade-guide-4_24.adoc` that says this and points to `uriTemplate`?
> 4. [Nit] The last commit message has a typo ("anf"). It disappears if the
PR is squashed.
>
> About the red `build (21, false)`: it is not caused by this PR. The
failure is `OBSConsumerInProgressTest.deletedObjectIsSkipped` in
`camel-huaweicloud-obs`. That test ran only because the catalog changed, which
makes CI test every dependent module, and this PR touches nothing in OBS or in
the polling consumer code. The same test passed in another full-build run on
2026-11-12. `camel-ai-resource`, `camel-mcp-server-api` and `camel-mcp-server`
all ran and passed in this run. A re-run, or `gh pr update-branch 29722
--rebase`, should turn it green.
>
> Once point 1 and the imports are done, this looks good to go from my side.
>
> _Claude Code on behalf of davsclaus. This review was generated by an AI
agent and may contain inaccuracies; please verify all suggestions before
applying._
The three requested changes are addressed
--
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]
