Repository navigation
feat(starters): add AgentBuilderCustomizer and auto-assembly extensions - #2488
dracula337435 wants to merge 1 commit into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
664469c to
4768685
Compare
AgentScopeJavaBot
added
enhancement
4768685 to
2aca2e7
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds an AgentBuilderCustomizer SPI to the core Spring Boot starter plus @ToolBean tool auto-registration, middleware/hook/permission auto-assembly and docs for EN+ZH. The direction is good — declarative agent assembly via ObjectProvider + orderedStream(), deprecated Hook plumbing isolated so it can be deleted with the API, and both doc languages updated. It is not ready to approve yet: the marker annotation does not match its own documented usage, and there are two auto-configuration robustness problems (bean resolution inside a BeanPostProcessor, and a strict getIfAvailable() on a multi-eligible bean) plus an ordering guarantee the PR relies on but does not establish.
Findings
- [Critical]
ToolBean.java:51—@ToolBeanis not a stereotype, so the bare-annotation usage documented in@ToolBeanjavadoc,ToolAutoRegistrationBeanPostProcessorjavadoc anddocs/v2/{en,zh}/building-blocks/tool.mdregisters nothing. - [Warning]
ToolAutoRegistrationBeanPostProcessor.java:56—getBeansWithAnnotationinsidepostProcessAfterInitializationforces tool-bean instantiation during another bean's initialization (startup-cycle risk with prototypeToolkit), and no de-dup against manually registered tools. - [Critical]
AgentscopeAutoConfiguration.java:196—getIfAvailable()throws on multiplePermissionContextStatebeans; the customizer is documented as a no-op when absent. - [Warning]
AgentscopeAutoConfiguration.java:178— auto customizers have no@Order, so user customizers are overridden silently and middleware order is undefined. - [Warning]
AgentBuilderCustomizer.java:41— SPI shape differs fromOpenAIChatModelBuilderCustomizer(customize()+ defaultaccept()) that the javadoc claims to mirror. - [Info]
HookAutoConfiguration.java:45— auto-attaching existingHookbeans is a behaviour change; worth calling out in the description/notes.
Suggestions
- Make
@ToolBeanmeta-annotated with@Component(preferred, matches the docs) or fix all three doc sites to show@Component @ToolBean/ an explicit@Bean, then add a test that uses the doc example verbatim. - Replace
getIfAvailable()withgetIfUnique(); add a two-PermissionContextState-bean test. - Annotate
middlewareAutoCustomizer/permissionContextAutoCustomizer/hookAutoCustomizerwith an explicit@Orderand test user-vs-auto precedence. - Declare
void customize(ReActAgent.Builder)with a defaultacceptdelegate so the starter SPI family is uniform.
CLA signed, CI green, no merge conflicts. Blocking items above are all small, localized changes.
Automated review by github-manager-bot
| @Target(ElementType.TYPE) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Documented | ||
| public @interface ToolBean {} |
There was a problem hiding this comment.
[Critical] @ToolBean is a bare marker (@Target(TYPE)/@Retention(RUNTIME)), not meta-annotated with @Component. So the usage documented here (and in docs/v2/{en,zh}/.../tool.md + ToolAutoRegistrationBeanPostProcessor javadoc) — annotate a class with only @ToolBean and it gets auto-registered — does not work: ApplicationContext#getBeansWithAnnotation only finds beans that already exist, and a class carrying no stereotype never becomes one. The test passes only because ToolBeanConfiguration additionally declares @Bean TestTools testTools().
Please pick one and make code + both doc pages agree:
- make it a stereotype —
@Componentmeta-annotation on@ToolBean(then component scanning finds it, and the doc example works as written); or - keep it a marker and show
@Component @ToolBean(or an explicit@Bean) in every example.
Option 1 matches the "declare a bean and it just works" promise in the PR description; option 2 is the smaller API surface. Either way a regression test that mirrors the doc example verbatim (bare @ToolBean class, no @Bean method) would have caught this.
| throws BeansException { | ||
| if (bean instanceof Toolkit toolkit) { | ||
| Map<String, Object> toolBeans = | ||
| applicationContext.getBeansWithAnnotation(ToolBean.class); |
There was a problem hiding this comment.
[Warning] Resolving beans from inside postProcessAfterInitialization is a bootstrap-order hazard. Toolkit is prototype-scoped here, so every ObjectProvider<Toolkit>.getObject() walks all @ToolBean definitions and forces their instantiation while another bean is still being initialized. A @ToolBean that injects the Toolkit (a natural thing for a tool that composes other tools) then hits a bean-in-creation cycle, and the failure only shows up at context startup — which the happy-path test cannot see.
Also, nothing here de-duplicates against tools the application registered manually: registerTool on an existing name is currently a silent overwrite (that is exactly what #3332 proposes to change), so a user who both declares @ToolBean and calls toolkit.registerTool(sameObject) gets an error after #3332 merges. Consider (a) registering on postProcessBeforeInitialization / deferring the lookup to the agent-build customizer instead of the BPP, and (b) skipping a tool whose name is already present in the toolkit, with a debug log.
| public AgentBuilderCustomizer permissionContextAutoCustomizer( | ||
| ObjectProvider<PermissionContextState> permissionContext) { | ||
| return builder -> { | ||
| PermissionContextState ctx = permissionContext.getIfAvailable(); |
There was a problem hiding this comment.
[Critical] getIfAvailable() throws NoUniqueBeanDefinitionException when more than one PermissionContextState bean is present, so a legitimate multi-context setup (e.g. a default plus a per-tenant / per-session profile in the distribution module) would fail startup with an auto-configuration that is documented as "no-op when absent". Use getIfUnique() and log at debug when it resolves to nothing, or fail with a message that names the conflicting beans.
| @ConditionalOnMissingBean(name = "middlewareAutoCustomizer") | ||
| public AgentBuilderCustomizer middlewareAutoCustomizer( | ||
| ObjectProvider<MiddlewareBase> middlewares) { | ||
| return builder -> middlewares.orderedStream().forEach(builder::middleware); |
There was a problem hiding this comment.
[Warning] These built-in customizers carry no @Order, so with orderedStream() they all land on Ordered.LOWEST_PRECEDENCE and ties fall back to bean-definition order. Auto-configuration definitions are registered after user configuration, so a user's own unordered AgentBuilderCustomizer runs first and is then silently overridden by permissionContextAutoCustomizer / middlewareAutoCustomizer — and for middleware the injection order is behaviour, not cosmetics (hook ordering in the ReAct loop). Please give the auto customizers an explicit, documented order (e.g. @Order(Ordered.HIGHEST_PRECEDENCE) so user customizers can always win), and add a test with two customizers — one user, one auto — asserting the documented precedence for permissionContext(...) and middleware(...).
| * @see AgentscopeAutoConfiguration#agentscopeReActAgent | ||
| */ | ||
| @FunctionalInterface | ||
| public interface AgentBuilderCustomizer extends Consumer<ReActAgent.Builder> {} |
There was a problem hiding this comment.
[Warning] The javadoc says this "mirrors the existing ChatModelBuilderCustomizer pattern used by model provider starters", but it does not: OpenAIChatModelBuilderCustomizer (and siblings) declare their own void customize(Builder) and only implement accept as a default delegate, and OpenAIAutoConfiguration invokes customizer.customize(builder). This interface inherits accept as its single abstract method, so the two starter families end up with different SPI shapes for the same concept — confusing for users implementing both, and it makes the @FunctionalInterface claim depend entirely on java.util.function.Consumer staying the super-interface.
Please either declare void customize(ReActAgent.Builder) + default void accept(...) to truly match the existing pattern (and call customize in agentscopeReActAgent), or drop the "mirrors" wording. A starter-wide *BuilderCustomizer convention note in the docs would help the next extension too.
| public class HookAutoConfiguration { | ||
|
|
||
| /** | ||
| * Auto-injects all {@link Hook} beans into the agent builder, ordered by |
There was a problem hiding this comment.
[Info] Isolating the deprecated Hook plumbing in its own auto-configuration is the right call and the javadoc explaining it is appreciated. Two smaller points: this file ships without a license header check concern only in that it is the first auto-configuration in the starter activated purely by AutoConfiguration.imports, so @ConditionalOnClass(Hook.class) plus @ConditionalOnProperty(...) is what keeps it off for users without hooks — worth stating that in the PR description as a behaviour change ("existing Hook beans are now auto-attached when agentscope.agent.enabled=true"), since previously nothing was attached implicitly. If a release-notes/CHANGELOG entry is expected for starter behaviour changes, this and the @ToolBean registration both need one.
There was a problem hiding this comment.
Thanks for the detailed review — every finding is valid. Here is how each was addressed in
the updated commit dcd2ab72.
[Critical] ToolBean.java:51 — @ToolBean is not a stereotype
Confirmed. getBeansWithAnnotation only sees beans that already exist, so a bare @ToolBean
class never becomes one — the documented usage (and the test, which passed only because of the
extra @Bean method) was wrong.
Rather than paper over it by adding @Component, I removed the whole tool auto-registration
path (@ToolBean + ToolAutoRegistrationBeanPostProcessor), its tests, and the tool docs.
Rationale: auto tool scan is already tracked by #821. #1192 attempted it and was closed;
its description shows the blocker is that Spring AOP proxies (CGLIB/JDK) drop
@Tool/@ToolParam annotations, so the fix requires targetClass-aware scanning in core
Toolkit/ToolMethodInvoker — not something a starter-level PR can do correctly. #626
(custom annotation on an @Tool method prevents registration) points at the same
annotation-handling area. Deferring to #821 keeps this PR free of that dependency.
This also removes the [Warning] against ToolAutoRegistrationBeanPostProcessor.java:56 — the
bootstrap-order hazard and the missing de-dup no longer exist.
[Critical] AgentscopeAutoConfiguration.java:196 — getIfAvailable() throws on multiple beans
Fixed. Now uses getIfUnique(), with a debug log when it resolves to nothing. Added
shouldNotFailWhenMultiplePermissionContextStateBeans, which registers two
PermissionContextState beans and asserts the context starts and the mode stays at the
default (no auto-injection, no NoUniqueBeanDefinitionException).
[Warning] AgentscopeAutoConfiguration.java:178 — no @Order on auto customizers
Fixed. middlewareAutoCustomizer, permissionContextAutoCustomizer, and hookAutoCustomizer
are all annotated @Order(Ordered.HIGHEST_PRECEDENCE) and documented as such, so user-defined
AgentBuilderCustomizer beans always run afterwards and win.
Added two precedence tests:
userCustomizerShouldOverrideAutoInjectedPermissionContext— auto injectsACCEPT_EDITS,
a user customizer overwrites withBYPASS, final mode isBYPASS.userCustomizerShouldRunAfterAutoMiddlewareCustomizer— asserts the user middleware appears
after the auto-injected one inagent.getMiddlewares().
[Warning] AgentBuilderCustomizer.java:41 — SPI shape differs from OpenAIChatModelBuilderCustomizer
Fixed. The interface now declares void customize(ReActAgent.Builder) and a
default void accept(...) that delegates to it, exactly matching the model customizer family,
and agentscopeReActAgent invokes customize(builder). The javadoc no longer claims a
"mirror" that was not there — it is now the same shape.
[Info] HookAutoConfiguration.java:45 — implicit hook attach is a behaviour change
Agreed and now called out explicitly in the class javadoc and in the PR description: previously
nothing was attached implicitly; with agentscope.agent.enabled=true every Hook bean is
auto-attached. The javadoc also notes that applications attaching hooks manually should drop
the manual attachment to avoid double registration. Flagged for release notes.
Validation
| Check | Result |
|---|---|
mvn spotless:apply |
Pass |
mvn test -pl .../agentscope-spring-boot-starter |
Pass — 12 run, 0 failures, 0 errors |
| Public API change | Additive only; no customizer beans → behaviour unchanged |
8d88145 to
9aa3825
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds an AgentBuilderCustomizer SPI to the core Spring Boot starter and uses it for middleware auto-assembly, PermissionContextState auto-injection and (in a separate, deliberately isolated auto-configuration) deprecated Hook auto-assembly, with EN+ZH docs and @WebMvcTest-style context tests. Compared with the previous head this drops the @ToolBean auto-registration entirely and replaces getIfAvailable() with getIfUnique(), which resolves both critical findings from the last round — that is the right call and the remaining surface is much smaller. Holding at comment level on three behaviour/visibility issues and one ordering guarantee that is implied but not declared.
Findings
- [Warning]
AgentscopeAutoConfiguration.java:168— auto-assembly is additive (Builder.middleware()just appends), so an app that still attaches the same middleware itself now runs it twice; the only escape hatch is shadowing a bean literally namedmiddlewareAutoCustomizer. - [Warning]
AgentscopeAutoConfiguration.java:191—getIfUnique()collapses "no bean" and "two or more beans" into onedebugline; in the ambiguous case the agent silently runs on the default permission context. - [Warning]
HookAutoConfiguration.java:61— auto-attaching everyHookbean is a silent behaviour change for a deprecated API, and the javadoc asks existing apps to delete their manualbuilder.hook(...)calls; no property opt-out and no migration note in the docs. - [Info]
AgentscopeAutoConfiguration.java:163— all three built-in customizers shareHIGHEST_PRECEDENCE, so their mutual order is registration order, not declared order. - [Info]
AutoConfiguration.imports:17— the two auto-configurations have no declaredafter/before, so customizer sequence depends on alphabetical class ordering.
Suggestions
- Prefer
logger.warn(notdebug) when more than onePermissionContextStatecandidate exists — the permission engine decides allow/approve/deny, so a silently dropped context is a security-relevant misconfiguration, not a no-op. Distinguish it withorderedStream().toList()andsize(). - Add a property switch for each auto-assembly (
agentscope.agent.auto-assemble-middleware,...-hooks) defaulting to the current behaviour, so users who wire manually have a documented opt-out instead of a bean-name collision.@ConditionalOnProperty(matchIfMissing = true)composes fine with the existingagent.enabledgate. - Note the double-attach risk in
docs/v2/{en,zh}/building-blocks/middleware.mdand add the equivalent hook note next to the Hook docs — the class javadoc is the only place it currently appears. A test for "middleware bean already attached manually" would pin the semantics either way. - Give the built-in customizers distinct order values (
HIGHEST_PRECEDENCE + 10/20/30) or state in the javadoc that their relative order is unspecified; the "user-defined customizers always run last" guarantee does hold for unordered user beans (they default toLOWEST_PRECEDENCE).
CI on this head: validate, Check License and Check Module Sync pass, both build jobs are still running — re-request once they are green.
Automated review by github-manager-bot
| @ConditionalOnMissingBean(name = "middlewareAutoCustomizer") | ||
| public AgentBuilderCustomizer middlewareAutoCustomizer( | ||
| ObjectProvider<MiddlewareBase> middlewares) { | ||
| return builder -> middlewares.orderedStream().forEach(builder::middleware); |
There was a problem hiding this comment.
Auto-assembly is additive, so an application that already wires a middleware now gets it twice. ReActAgent.Builder.middleware(MiddlewareBase) is just this.middlewares.add(middleware) with no de-duplication (same for hook(...)), and this customizer runs before any user customizer. An app that had builder.middleware(myMiddleware()) in its own AgentBuilderCustomizer — the documented pre-PR pattern — now runs that middleware twice per call: double spans for tracing middleware, double retry wrapping, duplicated permission checks. shouldAutoInjectMiddlewareBeans covers the happy path but not the already-attached case.
Either skip instances the builder already holds, or make the escape hatch a property instead of a bean name:
@ConditionalOnProperty(prefix = "agentscope.agent", name = "auto-assemble-middleware", matchIfMissing = true)Today the only opt-out is @ConditionalOnMissingBean(name = "middlewareAutoCustomizer"), i.e. defining a bean literally called middlewareAutoCustomizer — nothing in the docs or an error message points a user there.
| public AgentBuilderCustomizer permissionContextAutoCustomizer( | ||
| ObjectProvider<PermissionContextState> permissionContext) { | ||
| return builder -> { | ||
| PermissionContextState ctx = permissionContext.getIfUnique(); |
There was a problem hiding this comment.
getIfUnique() collapses two very different situations into one silent debug line: "no PermissionContextState bean at all" (the documented, expected case — shouldNotRequirePermissionContextStateBean) and "two or more candidates, so the permission context the application intended is dropped" (a misconfiguration). The second decides whether tool calls go through allow/approve/deny, so it should not be invisible at the default log level:
List<PermissionContextState> candidates = permissionContext.orderedStream().toList();
if (candidates.size() == 1) {
builder.permissionContext(candidates.get(0));
} else if (candidates.size() > 1) {
logger.warn("Found {} PermissionContextState beans; auto-injection skipped, "
+ "agent falls back to the default permission context", candidates.size());
}shouldNotFailWhenMultiplePermissionContextStateBeans keeps passing — the skip just becomes visible. (Using getIfUnique() here instead of the previous getIfAvailable() is the right fix for the throw flagged last round.)
| @Order(Ordered.HIGHEST_PRECEDENCE) | ||
| @ConditionalOnMissingBean(name = "hookAutoCustomizer") | ||
| public AgentBuilderCustomizer hookAutoCustomizer(ObjectProvider<Hook> hooks) { | ||
| return builder -> hooks.orderedStream().forEach(builder::hook); |
There was a problem hiding this comment.
Same double-attachment exposure as the middleware customizer, but here the class javadoc explicitly tells existing applications to delete their manual builder.hook(...) call — that is a silent behaviour change for an API already marked for removal. Two things would make this safer: (1) a property opt-out (agentscope.agent.auto-assemble-hooks) rather than shadowing a bean named hookAutoCustomizer; (2) @ConditionalOnBean(Hook.class) alongside @ConditionalOnClass so no customizer is registered at all when the context has no hooks. Since Hook is deprecated, keeping its new implicit behaviour switchable also shrinks the blast radius when the API is deleted.
| * {@link AgentBuilderCustomizer} runs afterwards and always wins. | ||
| */ | ||
| @Bean | ||
| @Order(Ordered.HIGHEST_PRECEDENCE) |
There was a problem hiding this comment.
All three built-in customizers (middlewareAutoCustomizer, permissionContextAutoCustomizer, and hookAutoCustomizer in the other auto-configuration) use the same Ordered.HIGHEST_PRECEDENCE, so their mutual order falls back to bean-definition order, which in turn comes from AutoConfiguration.imports plus method declaration order. That is fine today because they touch three different builder fields, but it is not stated anywhere. Either pin it (HIGHEST_PRECEDENCE + 10/20/30) or add one javadoc line saying the built-ins' relative order is unspecified. The guarantee that matters for users — an unordered AgentBuilderCustomizer sorts last — does hold, since missing order defaults to LOWEST_PRECEDENCE.
| # limitations under the License. | ||
| # | ||
| io.agentscope.spring.boot.AgentscopeAutoConfiguration | ||
| io.agentscope.spring.boot.HookAutoConfiguration |
There was a problem hiding this comment.
HookAutoConfiguration is appended with no after/before declared on the @AutoConfiguration annotation, so the two auto-configurations are sorted by class name. A before H happens to give the sequence the tests assert, but a rename or a future third customizer can flip it without any compile-time signal. @AutoConfiguration(after = AgentscopeAutoConfiguration.class) (or explicit order constants on the customizer beans, see the previous comment) would make the dependency declarative.
9aa3825 to
1cf9ff7
Compare
fca2d92 to
b71824e
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after the new commits on top of my earlier round. The AgentBuilderCustomizer SPI is a good match for the ChatModelBuilderCustomizer pattern, the back-off conditions and @Order layering are deliberate, and the added context tests cover both the opt-outs and the ordering guarantees. Overall the change looks sound — remaining items are about one security-relevant default and some behaviour-change documentation, not blockers.
Findings
- [Warning]
AgentscopeAutoConfiguration—PermissionContextStateauto-injection attaches one shared, session-scoped permission rule set to the singleton agent bean, which is a multi-tenant isolation risk. Recommend an explicit opt-in flag (default off), consistent withauto-assemble-middleware/auto-assemble-hooks. - [Warning]
AgentscopeAutoConfiguration— auto-assembled middlewares are always appended before user customizer middlewares, which silently reorders the hook chain for apps that relied on their ownbuilder.middleware(...)order. Please document@Orderon the middleware bean as the control knob. - [Info]
HookAutoConfiguration—enabledmust be set explicitly (havingValue="true", nomatchIfMissing) whileAgentProperties.enableddefaults totrue; the new flags inherit that trap. - [Info]
AgentscopeAutoConfigurationTest— stale comment (getIfUnique()vs the actual size-check path); the ambiguity warning branch is the security-relevant one and is worth asserting directly.
No CI failures and CLA is signed. Fine to proceed once the permission-context default is addressed or explicitly justified.
Automated review by github-manager-bot
| @Order(Ordered.HIGHEST_PRECEDENCE + 20) | ||
| @ConditionalOnProperty(prefix = "agentscope.agent", name = "enabled", havingValue = "true") | ||
| @ConditionalOnMissingBean(name = "permissionContextAutoCustomizer") | ||
| public AgentBuilderCustomizer permissionContextAutoCustomizer( |
There was a problem hiding this comment.
PermissionContextState is an immutable per-session/per-user rule set (allow/deny/ask rules + working directories), but this injects a single bean instance into the singleton ReActAgent, so every session in the context shares the same permission rules. That is a multi-tenant isolation risk for deployments that publish one shared agent. Suggest either requiring an explicit opt-in (agentscope.agent.auto-assemble-permission-context, default false) or documenting that a PermissionContextState bean must be treated as a global default only — unlike the middleware/hook assembly, there is currently no way to switch this one off.
| .model(model) | ||
| .toolkit(toolkit) | ||
| .maxIters(config.getMaxIters()); | ||
| customizers.orderedStream().forEach(c -> c.customize(builder)); |
There was a problem hiding this comment.
Because this customizer is ordered at HIGHEST_PRECEDENCE + 10, auto-assembled middlewares are always appended before any middleware added by a user-defined AgentBuilderCustomizer, which changes the effective hook order for apps that previously controlled ordering by their own builder.middleware(...) call order. Middleware ordering is behaviour-significant (short-circuit / tracing / permission hooks), so please state in the class javadoc and the docs snippet that auto-assembled middlewares run first and that @Order on the middleware bean is the knob to control relative order.
| havingValue = "true", | ||
| matchIfMissing = true) | ||
| @ConditionalOnMissingBean(name = "hookAutoCustomizer") | ||
| public AgentBuilderCustomizer hookAutoCustomizer(ObjectProvider<Hook> hooks) { |
There was a problem hiding this comment.
agentscope.agent.enabled must be set explicitly for this class to apply (havingValue="true" without matchIfMissing), while AgentProperties.enabled defaults to true. The mismatch is pre-existing for the agent bean, but the new auto-assemble-hooks/auto-assemble-middleware flags inherit the same trap: a user who sets only auto-assemble-hooks=false and never sets enabled gets the default behaviour silently. Consider matchIfMissing = true on the enabled condition (to match the field default) or aligning the docs on "explicitly required".
| SecondPermissionConfiguration.class) | ||
| .run( | ||
| context -> { | ||
| // getIfUnique() resolves to null on ambiguity: no exception, no |
There was a problem hiding this comment.
Test comment says getIfUnique() resolves to null on ambiguity, but the implementation uses orderedStream().toList() + size check. Worth updating the comment so the assertion documents the actual code path — and consider asserting the logger.warn branch is covered (e.g. via a ListAppender) since the "refuse to auto-inject an ambiguous permission context" behavior is the security-relevant one.
b71824e to
c05c863
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Reviewed the new push on this PR (c05c863b): a Spring Boot AgentBuilderCustomizer seam plus three built-in customizers (middleware auto-assembly, PermissionContextState auto-injection, and hook auto-assembly), with config flags, tests and docs. The design is the right shape — customization through a customizer instead of a growing auto-config method signature — and the test suite covers ordering, opt-out and the ambiguous-permission path. Findings are about implicit defaults and property surfacing rather than correctness blockers. CI: build (ubuntu/windows) and CLA are green, coverage still pending.
Findings
- [Warning]
HookAutoConfiguration.java:68— hook auto-assembly is on by default for a deprecated-for-removal API and silently double-registers hooks that apps already attach manually. - [Warning]
AgentscopeAutoConfiguration.java:179— context-wide middleware auto-assembly needs a documented statelessness/sharing contract (singleton middleware across prototype agents) and an escape from "every bean, regardless of which agent it was declared for". - [Warning]
AgentscopeAutoConfiguration.java:175— the twoauto-assemble-*keys are only read via@ConditionalOnProperty; noAgentPropertiesbinding and no configuration metadata, so a typo silently keeps the default. - [Info]
AgentscopeAutoConfiguration.java:204—getIfUnique()fits the "exactly one" rule; also the "user customizer always runs last" claim only holds for un-@Ordered beans. - [Info]
AgentBuilderCustomizer.java:37— extendingConsumeron top ofcustomizedoubles the API surface for no gain; cheaper to drop now than after 2.0.4.
Suggestions
- Flip
auto-assemble-hooksto opt-in (or de-duplicate against hooks already on the builder) and note the middleware/hook defaults in the release notes;docs/v2/*/middleware.mdalone will not be found by upgraders. - Add
autoAssembleMiddleware/autoAssembleHookstoAgentPropertiesso IDE completion and typo validation work on the opt-out switches. - Log the assembled middleware/hook list at
debugwhen the agent bean is created — that is the first thing anyone will need when an auto-attached middleware behaves unexpectedly.
Automated review by github-manager-bot
| prefix = "agentscope.agent", | ||
| name = "auto-assemble-hooks", | ||
| havingValue = "true", | ||
| matchIfMissing = true) |
There was a problem hiding this comment.
[Warning] matchIfMissing = true makes implicit Hook auto-assembly the default for an API that is deprecated for removal. Any application that already declares Hook beans and attaches them manually (the only supported path today) now registers each hook twice on the auto-configured agent, so hook side effects (tracing, recording, HITL prompts) fire twice with no warning at startup.
Since the Hook API is @Deprecated(forRemoval), growing its implicit usage surface seems backwards. Suggest either matchIfMissing = false (opt-in, consistent with "new code should use middleware"), or de-duplicating against hooks already attached to the builder. At minimum please call this behaviour change out in the release notes, not only in the class javadoc.
| @ConditionalOnMissingBean(name = "middlewareAutoCustomizer") | ||
| public AgentBuilderCustomizer middlewareAutoCustomizer( | ||
| ObjectProvider<MiddlewareBase> middlewares) { | ||
| return builder -> middlewares.orderedStream().forEach(builder::middleware); |
There was a problem hiding this comment.
[Warning] middlewares.orderedStream() collects every MiddlewareBase bean in the context and attaches it to the auto-configured agent. Two follow-ups worth documenting:
- Instance sharing. A singleton middleware is now shared by all agents built from this configuration (and by prototype agents re-created per injection point). If any middleware keeps per-call state (conversation buffers, counters, tenant ids), this becomes a cross-session leak. Worth an explicit javadoc requirement that auto-assembled middleware must be stateless/thread-safe — the
Memorybean above already documents its own prototype-scoping, so the same treatment here would be consistent. - Scope. In an app that also builds its own agents (harness sub-agents, other
ReActAgentbeans), a middleware declared for one agent is now implicitly applied to the starter agent as well. Please document that auto-assembly is context-wide, or key it off a qualifier.
| prefix = "agentscope.agent", | ||
| name = "auto-assemble-middleware", | ||
| havingValue = "true", | ||
| matchIfMissing = true) |
There was a problem hiding this comment.
[Warning] auto-assemble-middleware / auto-assemble-hooks are read only through @ConditionalOnProperty, so they are invisible to configuration binding: AgentProperties has no matching fields and the starter has no additional-spring-configuration-metadata.json. Users get no IDE completion and no validation of typos (auto-assemble-middlewarea=false silently keeps the default), which is a bad failure mode for an opt-out switch intended as an escape hatch.
Please add boolean autoAssembleMiddleware = true / autoAssembleHooks to AgentProperties (or metadata entries) so the two new keys are part of the documented property surface, and reference them from the docs/v2/*/middleware.md sections added here.
| @ConditionalOnMissingBean(name = "permissionContextAutoCustomizer") | ||
| public AgentBuilderCustomizer permissionContextAutoCustomizer( | ||
| ObjectProvider<PermissionContextState> permissionContext) { | ||
| return builder -> { |
There was a problem hiding this comment.
[Info] ObjectProvider<T>.getIfUnique() expresses exactly the "exactly one bean, otherwise do nothing" rule here and avoids materializing the whole stream on every agent build; the size() > 1 warn branch then becomes getIfUnique() == null plus an optional stream().count() for the log message.
Also note the ordering guarantee is subtler than the javadoc suggests: orderedStream() puts un-@Ordered user customizers at LOWEST_PRECEDENCE, so a user customizer that does implement Ordered/@Order(HIGHEST_PRECEDENCE) runs before these built-ins and its permissionContext(...) will be overwritten. Worth one sentence in the class javadoc.
| */ | ||
| @FunctionalInterface | ||
| public interface AgentBuilderCustomizer extends Consumer<ReActAgent.Builder> { | ||
|
|
There was a problem hiding this comment.
[Info] This mirrors the Boot idiom (WebServerFactoryCustomizer) except that it both extends Consumer<ReActAgent.Builder> and declares customize. The accept default keeps the two in sync today, but it makes the interface assignable to Consumer, so a plain Consumer<ReActAgent.Builder> lambda can no longer be distinguished from a customizer and the ordering contract silently applies to both. Consider dropping extends Consumer and keeping customize only — cheaper to do now than after 2.0.4 ships.
c05c863 to
deddb43
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR adds an AgentBuilderCustomizer SPI to agentscope-spring-boot-starter and uses it for auto-assembly of MiddlewareBase beans (on by default, opt-out), a unique PermissionContextState bean, and opt-in Hook beans via a new, isolated HookAutoConfiguration. Existing beans and conditions are preserved, the new config keys are documented in metadata plus EN/ZH docs, and the added tests assert real agent state rather than context-load smoke. Verdict: sound and consistent with the sibling ChatModelBuilderCustomizer pattern; only minor documentation/cleanup notes remain.
Round status
- [Q1 customizer ordering / application point] resolved —
AgentBuilderCustomizeris a@FunctionalInterface(AgentBuilderCustomizer.java:35, abstractcustomizeat:43with a defaultacceptat:51) applied viacustomizers.orderedStream().forEach(c -> c.customize(builder))atAgentscopeAutoConfiguration.java:152, immediately after the starter defaults (:145-151) and just beforebuild()(:153); order is deterministic becauseorderedStream()sorts byOrdered/@Order, with the built-ins pinned atHIGHEST_PRECEDENCE + 10/20/30(:177,:218,HookAutoConfiguration.java:65). - [Q2 builder invariant safety] resolved — customizers get the live, fully mutable builder and can override or clear anything the starter set; this is the documented intent (
AgentscopeAutoConfiguration.java:78-82,docs/v2/*/building-blocks/middleware.md:79) so that user customizers always win. No validation guard exists (build()copiestoolkitatReActAgent.java:5663andmodelatReActAgent.java:334without null checks), and middleware can only be added, never removed, so the only escape hatches areagentscope.agent.auto-assemble-middleware=falseor name-shadowing (:172-184) — acceptable for a standard Boot customizer SPI, worth one javadoc line at most. - [Q3 hook auto-configuration conditionals] resolved — the class is gated by
@ConditionalOnClass(Hook.class)andagentscope.agent.enabled=true(HookAutoConfiguration.java:51-52) while the bean is opt-in viaauto-assemble-hooks=truewith nomatchIfMissingplus@ConditionalOnMissingBean(name = "hookAutoCustomizer")(:66-70), so users who define their own hooks but do not opt in are untouched; with zero hook beans the lambda is a silent no-op (:73-81), and ordering is preserved in registration order (orderedStream(),:73) with execution then followingHook.priority()(ascending, default 100, ties keep registration order —Hook.java:31-32,:175-176). - [Q4 backward compatibility of changed auto-config beans] resolved —
agentscopeMemory()/agentscopeToolkit()are untouched (AgentscopeAutoConfiguration.java:103,:121) andagentscopeReActAgentkeeps its return type and all conditions including@ConditionalOnMissingBean(:134-137); the only signature change is the container-resolved 5th parameter (:143), and the two new@Beans are a brand-new type (AgentBuilderCustomizer) gated by name-based@ConditionalOnMissingBean, so they cannot shadow user beans. One behavior change is worth noting: middleware auto-assembly is default-on (:179-183), so an app that declaresMiddlewareBasebeans will now have them attached to the starter-builtReActAgentwithout opting in (duplicate hazard documented at:167-174). - [Q5 metadata / tests / docs parity] resolved —
additional-spring-configuration-metadata.json:4and:10matchAgentProperties.java:60(boolean autoAssembleMiddleware = true) and:68(= false) in name, boxedBooleantype, and default; the new tests are real assertions on agent state (AgentscopeAutoConfigurationTest.java:111,:127,:140,:154,:176,:189,:201,:219;HookAutoConfigurationTest.java:54,:65); and EN/ZH docs add the same Tip block with the shipped keysagentscope.agent.auto-assemble-middleware/agentscope.agent.auto-assemble-hooks(docs/v2/en/.../middleware.md:79,docs/v2/zh/.../middleware.md:79) — the "mirrorsChatModelBuilderCustomizer" claim also checks out against the provider starters.
Findings
- [Info]
AgentscopeAutoConfiguration.java:140— the injectedMemory memoryparameter is never used (nomemory(...)onReActAgent.Builder), contradicting the javadoc at:128; drop it or document how memory is meant to reach the agent. - [Info]
additional-spring-configuration-metadata.json:4— duplicates entries already generated byspring-boot-configuration-processor(pom line 55) fromAgentProperties, creating a second source of truth that can drift; prefer removing the file. - [Info]
AgentscopeAutoConfiguration.java:184— all three built-in customizers back off by exact bean name but onlymiddlewareAutoCustomizeris documented; documentpermissionContextAutoCustomizerandhookAutoCustomizer(or switch to type-based conditions) so the override seam is discoverable.
CI / tests
CI on head deddb436df is green (Check License, Check Module Sync, validate, build (ubuntu-latest), build (windows-latest), codecov/patch all pass) and license/cla is signed. Test execution was not performed in this review round.
Automated review by github-manager-bot
| public ReActAgent agentscopeReActAgent( | ||
| Model model, Memory memory, Toolkit toolkit, AgentscopeProperties properties) { | ||
| Model model, | ||
| Memory memory, |
There was a problem hiding this comment.
[INFO] The injected Memory memory parameter is still never used: the new builder chain (lines 145-151) sets name/sysPrompt/model/toolkit/maxIters but no memory, and ReActAgent.Builder exposes no memory(...) method, so neither the auto-configuration nor an AgentBuilderCustomizer can apply the agentscopeMemory() bean. This also contradicts the method javadoc (line 128: "wires together the configured Model, Memory and Toolkit beans") and the memory-bean javadoc (lines 96-99) that recommends obtaining it via method injection. Action: either drop the parameter (and the now-unconsumed Memory bean contract) or document explicitly where application code is expected to supply memory (e.g. via AgentState). Pre-existing, but this PR rewrote this method body and added the customizer SPI on top of it.
| { | ||
| "properties": [ | ||
| { | ||
| "name": "agentscope.agent.auto-assemble-middleware", |
There was a problem hiding this comment.
[INFO] These two entries duplicate metadata that spring-boot-configuration-processor already generates from AgentProperties (processor dependency present at agentscope-spring-boot-starter/pom.xml:55; the new fields at AgentProperties.java:60 and :68 are reachable from @ConfigurationProperties(prefix = "agentscope")). The merge of additional metadata over generated metadata is harmless today, but the names/types/defaults/descriptions now live in two places and can silently drift (e.g. a future default change in AgentProperties not mirrored here, or vice versa). Action: delete this file and rely on the generated metadata, or add a comment/check noting why it is needed.
| name = "auto-assemble-middleware", | ||
| havingValue = "true", | ||
| matchIfMissing = true) | ||
| @ConditionalOnMissingBean(name = "middlewareAutoCustomizer") |
There was a problem hiding this comment.
[INFO] All three built-in customizers back off only by exact bean name (middlewareAutoCustomizer here, permissionContextAutoCustomizer at line 220, hookAutoCustomizer in HookAutoConfiguration.java:70), yet only the middleware case is documented (javadoc at line 174, and no mention in docs/v2/{en,zh}/.../middleware.md). Users therefore cannot discover how to replace the permission or hook assembly, and an unrelated bean that happens to carry one of these names silently disables the built-in customizer. Action: document the three shadowing bean names in the reference docs (or switch the conditions to type-based @ConditionalOnMissingBean where a single seam type is intended).
deddb43 to
a8b984e
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed after the force-push (deddb436 -> a8b984ef). The delta is small and all of it is about metadata and documentation, not behaviour: two javadoc notes stating that permissionContextAutoCustomizer and hookAutoCustomizer back off via @ConditionalOnMissingBean(name = ...) when a user supplies a bean of that name (a real improvement — that escape hatch was previously invisible), @NestedConfigurationProperty on the agent / model groups in AgentscopeProperties, and deletion of the hand-written additional-spring-configuration-metadata.json. None of the five questions settled in the previous rounds is regressed, and the four core beans/conditions are unchanged. Holding at COMMENT rather than approval only because the configuration-hint story now rests entirely on annotation-processor output that nothing in CI verifies.
Findings
- [Warning]
AgentscopeProperties.java:38— the starter no longer ships explicit metadata foragentscope.agent.auto-assemble-middleware/auto-assemble-hooks; the entries now come from processor expansion of the nested groups. That is the right mechanism, but it is unverified in this repo:validate,Check Module Syncand the test suite never look atspring-configuration-metadata.json. Please paste the generated entries into the PR description, or keep the two hand-written entries as a fallback. The new javadoc's absolute claim ("without it the metadata contains noagentscope.agent.*entries") is also worth softening or backing with how it was measured, since Boot expands same-module nested types without the annotation. - [Info]
AgentProperties.java:60— with the hand-written metadata gone, this field javadoc is the only hint a user gets in the IDE, so the two facts that matter when the default-on behaviour surprises them (auto-assembly does not de-duplicate middleware an app already attaches; this flag is the opt-out) should live in that paragraph rather than only inAgentscopeAutoConfiguration's class javadoc. Same forautoAssembleHooks.
Status
CLA signed. Check License, Check Module Sync, validate, build (ubuntu-latest), build (windows-latest) all green on a8b984ef; codecov still reports 74% patch coverage with 13 lines uncovered, which was raised previously and is not a blocker here. No merge conflict.
@dracula337435 — thanks for the patience through several rounds; this is close. Once the generated metadata entries are visible (or the hand-written ones restored), this looks ready.
Automated review by github-manager-bot
| public class AgentscopeProperties { | ||
|
|
||
| private final AgentProperties agent = new AgentProperties(); | ||
| @NestedConfigurationProperty private final AgentProperties agent = new AgentProperties(); |
There was a problem hiding this comment.
This head deletes META-INF/additional-spring-configuration-metadata.json (it existed at the previously reviewed deddb436, absent here) and now relies entirely on the annotation processor expanding these nested groups because of @NestedConfigurationProperty. That is the cleaner shape, but it makes IDE hints for agentscope.agent.auto-assemble-middleware / auto-assemble-hooks — including their defaultValue and the "set false to avoid attaching the same middleware twice" guidance — an unverified side effect of the build: no check in validate, Check Module Sync or the test suite asserts on the generated spring-configuration-metadata.json. Please attach the generated fragment for those two keys in the PR description, or keep the two entries in the additional file as a belt-and-braces fallback. Also soften the new javadoc claim ("required ... without it the metadata contains no agentscope.agent.* entries"): Boot expands nested types from the same compilation unit without the annotation, so if that was the observed behaviour it is worth a sentence about how it was measured.
| * <p>Default {@code true}. Set to {@code false} when middleware is wired manually, to avoid | ||
| * attaching the same middleware twice. | ||
| */ | ||
| private boolean autoAssembleMiddleware = true; |
There was a problem hiding this comment.
Since the hand-written metadata entry is gone, this field javadoc is now the only source of the hint a user sees when the default bites them. Two facts are worth having in that one paragraph, because they are what a surprised applicant asks: that auto-assembly does not de-duplicate, so middleware already attached by the application or by another customizer is registered a second time; and that the opt-out is this single flag rather than removing the beans. Same note applies to autoAssembleHooks below.
a8b984e to
b41eb4d
Compare
- Add AgentBuilderCustomizer interface (customize() + default accept()), matching the ChatModelBuilderCustomizer SPI shape - Apply customizers to ReActAgent.Builder before build() via ObjectProvider - Add middlewareAutoCustomizer: auto-inject MiddlewareBase beans (default on; opt out with agentscope.agent.auto-assemble-middleware=false); logs the assembled bean classes at debug - Add permissionContextAutoCustomizer: auto-inject a unique PermissionContextState bean (warn and skip on ambiguity) - Add HookAutoConfiguration (isolated, Hook is @deprecated for removal): opt-in auto-attach via agentscope.agent.auto-assemble-hooks=true - Built-in customizers use distinct orders (HIGHEST_PRECEDENCE + 10/20/30) so their relative order is declared and user customizers without an explicit @order run last and can override - Mark the nested AgentscopeProperties groups with @NestedConfigurationProperty so the configuration metadata processor emits agentscope.agent.* and agentscope.model.* entries (IDE completion and typo validation)
b41eb4d to
a4a21c4
Compare
This branch has not been deployed
4 participants
AgentScope-Java Version
2.0.4-SNAPSHOT
Description
Background. The core Spring Boot starter (
agentscope-spring-boot-starter) creates aReActAgentwith onlyname/sysPrompt/model/toolkit/maxIters. Everything else —middleware, permission context, hooks — must be wired by hand, and a third-party extension
that wants to add one of these has no option but to replace the whole
ReActAgentbean.The model provider starters already solved the same problem with
ChatModelBuilderCustomizer; this PR brings the equivalent to the agent.Purpose. Add an
AgentBuilderCustomizerSPI plus two auto-assembly conveniences sothat declaring a
@Beanis enough.Changes.
AgentBuilderCustomizer(SPI) —@FunctionalInterfacedeclaringvoid customize(ReActAgent.Builder)with adefault void accept(...)delegate,matching the
OpenAIChatModelBuilderCustomizershape used by the model starters.AgentscopeAutoConfiguration#agentscopeReActAgentnow acceptsObjectProvider<AgentBuilderCustomizer>and applies every customizer beforebuild().Backward compatible: with no customizer beans the stream is empty and behaviour is unchanged.
middlewareAutoCustomizercollects everyMiddlewareBasebean (ordered by
@Order) and injects them into the builder.permissionContextAutoCustomizerapplies aunique
PermissionContextStatebean if one is present; no-op when absent or ambiguous(
getIfUnique()).HookAutoConfiguration(isolated so it can be deleted togetherwith the deprecated
Hook/HookEventAPI) auto-attaches everyHookbean.@Order(Ordered.HIGHEST_PRECEDENCE), so a user-definedAgentBuilderCustomizeralwaysruns afterwards and can override.
Behaviour change (please note). Previously no
Hookwas attached implicitly. Withagentscope.agent.enabled=true, everyHookbean is now auto-attached. Applications thatalready declare
Hookbeans and attach them manually should drop the manual attachment toavoid double registration. This should go into release notes for the starter.
Not included (deferred). Tool auto-registration (
@ToolBean) is not part of this PR.It is already tracked by #821; #1192 attempted it and was closed, and the AOP-proxy
annotation-loss it describes (CGLIB/JDK proxies dropping
@Tool) requires coreToolkit/ToolMethodInvokerchanges — it does not belong in a starter-level PR. Docs andtests for it were removed accordingly.
How to test.
mvn spotless:apply
mvn test -pl agentscope-extensions/agentscope-spring-boot-starters/agentscope-spring-boot-starter
12 tests cover: existing bean wiring, customizer application, middleware auto-injection,
user-vs-auto customizer precedence (permission and middleware), permission auto-injection,
absence of
PermissionContextState, ambiguity of multiplePermissionContextState, andhook auto-injection.
Checklist
mvn spotless:applymvn test)Generated configuration metadata
With
@NestedConfigurationPropertyon the nestedAgentscopePropertiesgroups,spring-configuration-metadata.jsonnow contains the following (previously thepropertiesarray was empty, so none of these had metadata). Verified withmvn clean compileand readingtarget/classes/META-INF/spring-configuration-metadata.json.agentscope.agent.auto-assemble-hooksBooleanfalseagentscope.agent.auto-assemble-middlewareBooleantrueagentscope.agent.enabledBooleantrueagentscope.agent.max-itersInteger10agentscope.agent.nameStringAssistantagentscope.agent.sys-promptStringYou are a helpful AI assistant.agentscope.model.providerStringnull