fix(nacos-starter): auto-configurations should be opt-in (matchIfMissing=false) and fix A2A server-addr override#1709
Conversation
…tNacosProperties for A2A merge - AgentscopeNacosPromptAutoConfiguration and AgentscopeA2aNacosAutoConfiguration both used matchIfMissing=true, causing them to activate even when the user had not configured the feature, resulting in a startup crash when the Nacos AI gRPC port (9848) was unreachable. - AgentscopeA2aNacosAutoConfiguration.a2aService() called getNacosProperties() on the A2A properties (which fills in the 127.0.0.1:8848 default) and then did a putAll on top of the base config, silently overwriting agentscope.nacos.server-addr. Fixed by using getExplicitNacosProperties() for the override, as BaseNacosProperties documents this method for exactly this overlay/merge use case. Fixes agentscope-ai#1707
AgentScopeJavaBot
added
bug
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR fixes two genuine bugs in agentscope-nacos-spring-boot-starter: (1) matchIfMissing=true incorrectly made opt-in auto-configurations activate by default, and (2) getNacosProperties() injected default serverAddr=127.0.0.1:8848 which silently overwrote user-configured base properties during merge. Both fixes are semantically correct and well-motivated. However, the PR does not update the existing tests in AgentscopeA2aNacosAutoConfigurationTest — at least 10 out of 14 tests rely on the old matchIfMissing=true behavior and will fail because they never set agentscope.a2a.nacos.enabled=true. These tests must be updated before merge.
| name = "enabled", | ||
| havingValue = "true", | ||
| matchIfMissing = true) | ||
| matchIfMissing = false) |
There was a problem hiding this comment.
[critical] Changing matchIfMissing to false is correct for the production code, but at least 10 existing tests in AgentscopeA2aNacosAutoConfigurationTest will break because they never set agentscope.a2a.nacos.enabled=true. Tests like shouldCreateDefaultBeansWhenEnabled, shouldCreateNacosAgentCardResolverBean, shouldCreateNacosAgentRegistryBean, shouldNotCreateNacosAgentCardResolverWhenDiscoveryDisabled, shouldNotCreateNacosAgentRegistryWhenRegistryDisabled, shouldBindNacosProperties, shouldCreateWithCustomAgentCardResolver, shouldCreateWithCustomAgentRegistry, shouldCreateBeansWithRegistryProperties, shouldCreateBeansWithDiscoveryProperties, and shouldCloseNacosServiceOnShutdown all rely on the old matchIfMissing=true behavior.
All positive test cases must be updated to add agentscope.a2a.nacos.enabled=true. Additionally, tests that expect AgentCardResolver or AgentRegistry beans must also set agentscope.a2a.nacos.discovery.enabled=true and/or agentscope.a2a.nacos.registry.enabled=true respectively, since those bean-level conditions also changed to matchIfMissing=false.
Example fix for shouldCreateDefaultBeansWhenEnabled:
.withPropertyValues(
"agentscope.a2a.nacos.enabled=true",
"agentscope.a2a.nacos.discovery.enabled=true",
"agentscope.a2a.nacos.registry.enabled=true",
"agentscope.a2a.nacos.server-addr=127.0.0.1:8848",
"agentscope.a2a.nacos.namespace=public")| throws NacosException { | ||
| Properties nacosClientProperties = nacosProperties.getNacosProperties(); | ||
| nacosClientProperties.putAll(a2aNacosProperties.getNacosProperties()); | ||
| nacosClientProperties.putAll(a2aNacosProperties.getExplicitNacosProperties()); |
There was a problem hiding this comment.
[praise] Good fix. Using getExplicitNacosProperties() instead of getNacosProperties() is the correct approach for the overlay/merge pattern — it avoids injecting default serverAddr=127.0.0.1:8848 which would silently overwrite the user-configured agentscope.nacos.server-addr via putAll. This is now consistent with AgentscopeNacosPromptAutoConfiguration (line 68) which already uses getExplicitNacosProperties().
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR fixes two genuine bugs in agentscope-nacos-spring-boot-starter: (1) matchIfMissing=true incorrectly made opt-in auto-configurations activate by default, and (2) getNacosProperties() injected default serverAddr=127.0.0.1:8848 which silently overwrote user-configured base properties during merge. Both fixes are semantically correct and well-motivated. However, the PR does not update the existing tests in AgentscopeA2aNacosAutoConfigurationTest — at least 10 out of 14 tests rely on the old matchIfMissing=true behavior and will fail because they never set agentscope.a2a.nacos.enabled=true. These tests must be updated before merge.
| name = "enabled", | ||
| havingValue = "true", | ||
| matchIfMissing = true) | ||
| matchIfMissing = false) |
There was a problem hiding this comment.
[critical] Changing matchIfMissing to false is correct for the production code, but at least 10 existing tests in AgentscopeA2aNacosAutoConfigurationTest will break because they never set agentscope.a2a.nacos.enabled=true. Tests like shouldCreateDefaultBeansWhenEnabled, shouldCreateNacosAgentCardResolverBean, shouldCreateNacosAgentRegistryBean, shouldNotCreateNacosAgentCardResolverWhenDiscoveryDisabled, shouldNotCreateNacosAgentRegistryWhenRegistryDisabled, shouldBindNacosProperties, shouldCreateWithCustomAgentCardResolver, shouldCreateWithCustomAgentRegistry, shouldCreateBeansWithRegistryProperties, shouldCreateBeansWithDiscoveryProperties, and shouldCloseNacosServiceOnShutdown all rely on the old matchIfMissing=true behavior.
All positive test cases must be updated to add agentscope.a2a.nacos.enabled=true. Additionally, tests that expect AgentCardResolver or AgentRegistry beans must also set agentscope.a2a.nacos.discovery.enabled=true and/or agentscope.a2a.nacos.registry.enabled=true respectively, since those bean-level conditions also changed to matchIfMissing=false.
Example fix for shouldCreateDefaultBeansWhenEnabled:
.withPropertyValues(
"agentscope.a2a.nacos.enabled=true",
"agentscope.a2a.nacos.discovery.enabled=true",
"agentscope.a2a.nacos.registry.enabled=true",
"agentscope.a2a.nacos.server-addr=127.0.0.1:8848",
"agentscope.a2a.nacos.namespace=public")| throws NacosException { | ||
| Properties nacosClientProperties = nacosProperties.getNacosProperties(); | ||
| nacosClientProperties.putAll(a2aNacosProperties.getNacosProperties()); | ||
| nacosClientProperties.putAll(a2aNacosProperties.getExplicitNacosProperties()); |
There was a problem hiding this comment.
[praise] Good fix. Using getExplicitNacosProperties() instead of getNacosProperties() is the correct approach for the overlay/merge pattern — it avoids injecting default serverAddr=127.0.0.1:8848 which would silently overwrite the user-configured agentscope.nacos.server-addr via putAll. This is now consistent with AgentscopeNacosPromptAutoConfiguration (line 68) which already uses getExplicitNacosProperties().
2 participants
AgentScope-Java Version
2.0.0-RC2
Description
Two bugs in agentscope-nacos-spring-boot-starter cause startup crashes and silent server-addr override.
matchIfMissing=true on @ConditionalOnProperty in AgentscopeNacosPromptAutoConfiguration and AgentscopeA2aNacosAutoConfiguration (and its nested discovery/registry beans) causes both auto-configurations to activate even when the user has not configured the feature. The Javadoc states these are opt-in features requiring enabled=true, but the implementation behaves as opt-out. Changed matchIfMissing to false.
AgentscopeA2aNacosAutoConfiguration.a2aService() calls a2aNacosProperties.getNacosProperties() when merging A2A-specific config on top of the base config. getNacosProperties() fills in 127.0.0.1:8848 as a default, which then overwrites the user-configured agentscope.nacos.server-addr via putAll. Fixed by using getExplicitNacosProperties() instead — BaseNacosProperties documents this method as the correct choice for overlay/merge scenarios.
To test: existing tests shouldNotCreateBeansWhenDisabled and shouldNotCreateNacosBeansWhenDisabled cover the matchIfMissing fix. The server-addr fix can be verified by running shouldNotOverwriteGlobalServerAddrWhenPromptNotSet in AgentscopeNacosPromptAutoConfigurationTest.
Fixes #1707