CAMEL-23950: complete AgentConfiguration coverage of AiServices builder - #25274
CAMEL-23950: complete AgentConfiguration coverage of AiServices builder#25274gnodet wants to merge 3 commits into
Conversation
Add the following configuration options to AgentConfiguration, wired in AbstractAgent.configureBuilder() with null guards: - inputGuardrails / outputGuardrails (List<InputGuardrail> / List<OutputGuardrail>): pre-instantiated guardrail instances for DI-friendly usage (Spring/Quarkus CDI), complementing the existing class-based guardrail support - inputGuardrailsConfig / outputGuardrailsConfig: guardrail configuration objects controlling retry policies and other guardrail behavior - beforeToolExecution / afterToolExecution (Consumer<BeforeToolExecution> / Consumer<ToolExecution>): hooks for per-tool-call logging, metrics, and tracing Deliberately not added: - maxSequentialToolsInvocations: deprecated alias for maxToolCallingRoundTrips (already exposed) since LangChain4j 1.15.0 - immediateReturnToolNames: no standalone AiServices method; already available via ToolProviderResult in the ToolProvider path Also improved aiServicesCustomizer Javadoc to document the chatRequestTransformer conflict with responseFormat/structured output, and listed builder options deliberately left for the escape hatch (toolSearchStrategy, storeRetrievedContentInChatMemory, AiServiceListener). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Follow JUnit 5 convention per project guidelines: new test methods use package-private visibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 tested, 29 compile-only — current: 10 all testedMaveniverse Scalpel detected 39 affected modules (current approach: 10).
|
gnodet
left a comment
There was a problem hiding this comment.
Looks good! Clean, well-structured PR that completes AgentConfiguration coverage of the LangChain4j AiServices builder with guardrail instances, guardrail configs, and tool execution hooks.
Highlights:
- Good decision to document the
chatRequestTransformerconflict in thewithAiServicesCustomizerJavadoc — this is a non-obvious footgun - The
@Experimentalwarnings onBeforeToolExecutionandToolExecutionset correct expectations for API stability - The "deliberately not added" rationale in the PR description (
maxSequentialToolsInvocationsas deprecated alias,immediateReturnToolNameswith no standalone builder method) shows good API curation judgment
Minor suggestion (non-blocking):
AgentConfigurationTestclass is still declaredpublic— per project conventions, thepublicmodifier should be dropped (the modifiedtestFluentChainingmethod and all new methods already use package-private correctly)
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the review! Fixed — dropped the Claude Code on behalf of gnodet |
gnodet
left a comment
There was a problem hiding this comment.
Re-review after new commit: the review feedback from the previous round (drop public from AgentConfigurationTest class declaration) has been correctly addressed. No other changes were made since the last review. LGTM ✅
Claude Code on behalf of Guillaume Nodet
gnodet
left a comment
There was a problem hiding this comment.
Review Summary (informational — not a formal approval)
Clean, well-structured PR that correctly extends AgentConfiguration to cover remaining AiServices builder options: guardrail instances, guardrail configs, and tool execution hooks. All new LangChain4j API types verified to exist in v1.18.0.
Key observations
-
API completeness: The six new LangChain4j API types (
InputGuardrail,OutputGuardrail,InputGuardrailsConfig,OutputGuardrailsConfig,BeforeToolExecution,ToolExecution) and their correspondingAiServicesbuilder signatures were confirmed in langchain4j 1.18.0. -
Good API curation: The "deliberately not added" rationale in the PR description (
maxSequentialToolsInvocationsas deprecated alias,immediateReturnToolNameswith no standalone builder method) demonstrates sound judgment. -
Valuable documentation: The
chatRequestTransformerconflict warning withresponseFormatin thewithAiServicesCustomizerJavadoc catches a non-obvious footgun that would silently break structured output. -
Clean ordering: New builder calls in
configureBuilderfollow logical grouping — guardrail instances after guardrail classes, configs after instances, tool hooks in the tool-calling section, andaiServicesCustomizeralways last as the escape hatch. -
@Experimentalwarnings: Appropriately noted onBeforeToolExecutionandToolExecutionin the Javadoc, since LangChain4j may change these APIs.
Minor notes (non-blocking)
- New test methods use JUnit assertions (consistent with existing file) rather than AssertJ. Reasonable choice for intra-file consistency.
- Setting guardrail configs without any corresponding guardrails would pass silently — a future enhancement could log a warning.
Note: This is an informational review only — an agent cannot approve its operator's own PRs. Human review is required.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
davsclaus
left a comment
There was a problem hiding this comment.
Claude Code on behalf of davsclaus
Clean, well-structured additive API extension. No blocking issues found.
Minor notes:
-
The Javadoc for
withInputGuardrails()states that guardrail classes and instances are "additive on the AiServices builder." BothinputGuardrailClasses()andinputGuardrails()are called independently inconfigureBuilder()— worth verifying this is truly additive in LangChain4j (rather than last-write-wins) if not already confirmed. -
The
@Experimentalwarning onBeforeToolExecution/ToolExecutionis well-documented in the Javadoc — just flagging that if LangChain4j changes these, an upgrade-guide entry would be needed.
Everything else looks good: logical placement in configureBuilder(), helpful cross-references in Javadoc, the chatRequestTransformer conflict warning on aiServicesCustomizer is valuable, and tests cover all 6 new fields.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
I have an architecture design and an issue following those JIRA issues aroudn langchain4j, can we revise them, before allowing anyone to contribute? |
zbendhiba
left a comment
There was a problem hiding this comment.
As a general rule: don't add anything to the langchain4j Agent SPI camel-langchain4j-agent-api without describing how it affects the camel-langchain4j-agent component. When you introduce configuration for concepts that already exist in the component (Tools, Guardrails), you need integration tests in the component and a design document explaining when to use one way or the other.
Every configuration added here scales to integration tests in sub-projects too camel-quarkus, camel-spring-boot`). Looking at the JIRA issues, the users requesting these features have deep LangChain4j knowledge. They could create the AiServices themselves. We should discuss whether we want to move to an AiService component instead, and stop adding all those configurations one by one.
|
@zbendhiba Thank you for the thoughtful review — these are valid architectural concerns. You're right that adding configuration fields one-by-one to the SPI scales into integration tests across sub-projects ( The broader design question — whether an Happy to adapt or close this PR based on the outcome. Claude Code on behalf of @gnodet |
Claude Code on behalf of gnodet
Summary
Completes
AgentConfigurationcoverage of the LangChain4jAiServicesbuilder API, following up on CAMEL-23928.Added
inputGuardrails/outputGuardrails): pre-instantiated guardrail objects for DI-friendly usage (Spring/Quarkus CDI), complementing the existing class-based support. This is a prerequisite for CAMEL-23344 (route-based guardrails that need aCamelContext-aware instance)inputGuardrailsConfig/outputGuardrailsConfig): config objects controlling retry policies and other guardrail behaviorbeforeToolExecution/afterToolExecution): callbacks invoked before/after each tool execution — the natural hook for per-tool-call logging, Camel events, Micrometer metrics and OTel spans. Foundation for CAMEL-23861Deliberately not added
maxSequentialToolsInvocations: deprecated alias formaxToolCallingRoundTrips(already exposed) since LangChain4j 1.15.0immediateReturnToolNames: no standaloneAiServicesbuilder method; already available viaToolProviderResultin theToolProviderpath (used by Camel route tools and MCP tools)Also
aiServicesCustomizerJavadoc to document thechatRequestTransformerconflict withresponseFormat/structured outputtoolSearchStrategy,storeRetrievedContentInChatMemory,AiServiceListener)Test plan
AgentConfigurationTest(getter/setter, fluent chaining, default values)testFluentChaining()to include all new fields🤖 Generated with Claude Code