Repository navigation
Conversation
- Read nacos.copilot.connections.<name>.* from the deployment environment. - Merge deployment connections with the legacy connection by normalized name, giving the deployment source precedence. - Validate each connection at load time and keep its diagnostic reason instead of dropping it, so one broken connection never hides the others. - Select the serving connection through CopilotModelRouter: explicit selection, then the first available deployment connection, then legacy. - Refresh the connection table on ServerConfigChangeEvent; only merged and standalone deployments watch the configuration file.
…resh The CopilotProperties bean is bound once at startup, so after a ServerConfigChangeEvent the fallback path kept returning stale selectedConnection/enabled/studioUrl values. Re-bind nacos.copilot on every refresh so all deployment fields, not just the connection table, take effect.
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
…resh off the event thread Sort connections with the same priority by name, because the property source does not preserve declaration order and the fallback would otherwise pick a different default after a reload. Warn once per refresh when the selected connection is missing, disabled or unavailable instead of falling back silently. Run the refresh on a dedicated executor so probing connections does not block the subscribers of ServerConfigChangeEvent, and initialize Studio only when its configuration changed.
The javadoc and spec text claimed that the legacy connection is reserved, that it requires an explicit apiKey, that only merged and standalone deployments watch the configuration file, and that a malformed deployment block is merely disabled. They now match the code: the legacy name can be overridden, the key comes from COPILOT_API_KEY or the stored apiKey, only merged deployments refresh, and a malformed block is dropped.
|
Please define the connection-name contract explicitly in the English and Chinese specs, then apply it consistently throughout the implementation and the subsequent user documentation. At
The issue discussion currently describes case-insensitive names. Please make the intended contract explicit, including allowed characters and leading/trailing whitespace handling:
This should cover deployment configuration, same-reader and cross-reader duplicates, connection lookup, and explicit selection. Step 3's Console/API behavior and the user-facing configuration documentation should carry the same contract, with clear examples. Please also distinguish any restrictions on configuration-key syntax from the case-sensitivity of connection identity. Please add tests for |
…yment blocks The reader passed the declared name straight to Spring's Binder, which rejects upper-case property names, so `Prod-Qwen` was dropped while the table normalized the same name for lookup. Blocks are now bound through a synthetic source and the normalized name is used as the identity, matching the merge, lookup and selectedConnection rules. Duplicate spellings of one name resolve deterministically: the lower-case spelling wins when present, otherwise the first in lexicographic order, and the others are reported and ignored. Both integration specs now state the connection-name contract and the configuration-key syntax restriction.
| if (studioUrl.equals(activeStudioUrl) && studioProject.equals(activeStudioProject)) { | ||
| LOGGER.debug("AgentScope Studio is already initialized for {}", studioUrl); | ||
| return; |
There was a problem hiding this comment.
[P2 / Must fix] Allow Studio recovery after a failed reconfiguration
This shortcut assumes that the last successfully initialized URL/project is still active, but a later failed initialization can invalidate that state.
Reproduction (keep the project unchanged):
- Configure Studio URL A and refresh; initialization succeeds, so
activeStudioUrlis cached as A. - Change the effective Studio URL to B and refresh. Let B fail during initialization (for example, the Studio endpoint is unavailable). In the bundled AgentScope 2.0.3, the initialization error path calls
StudioManager.shutdown(), clearing the actual Studio state. The catch block here only logs the exception, leaving the cached URL/project at A. - Restore URL A and refresh. This equality check matches the stale cache and returns before calling
initialize(), even thoughStudioManager.isInitialized()is now false. Further refreshes with A keep skipping recovery.
The result is that rolling back to a previously working Studio configuration does not restore the Studio integration. This concerns Studio recovery; it does not by itself demonstrate a Nacos startup or model-inference failure. The shortcut is new in step 2; step 1 retried initialization on refresh.
I reproduced the manager behavior with failure injection matching AgentScope's cleanup semantics: after A succeeds, B fails, and A is restored (including another refresh), initialization was attempted only twice and the final initialized state was false.
Please make the skip condition depend on valid current Studio state, and invalidate the cached successful configuration when a failed attempt clears that state. Add a regression test for A success -> B failure with shutdown -> A rollback success, while retaining the test that an unchanged, still-initialized configuration avoids redundant initialization.
| BindResult<CopilotProperties> bound = Binder.get(environment) | ||
| .bind(CONFIG_PREFIX, Bindable.of(CopilotProperties.class)); |
There was a problem hiding this comment.
[P2 / Must fix] Keep legacy configuration as a fallback when applying deployment selection
The intended upgrade contract is that the new deployment reader/configuration has higher priority; reading the old configuration is a compatibility fallback. The mere existence of copilot-config.json must not suppress explicitly configured new deployment values.
Currently, getEffectiveConfig() returns the entire stored object at line 247 before reaching this deployment rebind. Connection definitions still load independently through DeploymentConfigProviderReader, but the deployment-side selectedConnection never reaches the router.
Reproduction:
- Start from an upgraded installation where Nacos Config contains a valid
copilot-config.jsonin groupnacos-copilot, for example{"apiKey":"old-key","model":"qwen-turbo"}. Storage must be available and this JSON must be read successfully. The old JSON does not containselectedConnection. - Configure two enabled, available deployment connections,
alphaandbeta, undernacos.copilot.connections.*, and setnacos.copilot.selectedConnection=betain the deployment configuration. - Start Copilot or trigger a configuration refresh.
Expected: both deployment connections load and the explicit deployment selection routes to beta.
Actual: both connections load, but the effective selectedConnection is null from the old JSON. The router falls back to the first available deployment connection, alpha. Repeating the refresh does not help because the same stored-object early return is taken. With the old JSON absent, the same deployment configuration correctly selects beta.
Please fix the effective-configuration precedence so explicitly configured new deployment values take priority and legacy settings remain a fallback. Preserve legacy-only behavior and step 1's single storage read / atomic configuration-and-table snapshot. This is a source-precedence problem; the step 1 consistency fix is still valid.
Please also update both language specs and the configuration/migration documentation with this precedence, including how absent values fall back. Add regression coverage for an existing old JSON plus explicit deployment selection, changing that selection during refresh while the old JSON remains, conflicting values following the declared priority, and the legacy-only compatibility path.
What is the purpose of the change
Step 2 of the three-step plan agreed in #15906 (under the multi-provider track #15757). Step 1 (#15977) introduced the connection abstraction and mapped the existing configuration to a single
legacyconnection. This step adds the deployment-side connection table on top of it: connections declared undernacos.copilot.connections.*are read, merged with thelegacyconnection, validated at load time, and selected in the agreed order. When no deployment connection is configured, behavior is the same as after step 1.Brief changelog
DeploymentConfigProviderReader(order 10): enumerates connection names from the enumerable property sources undernacos.copilot.connections.and binds each block intoDeploymentConnectionDefinitionseparately, so one malformed block only drops that connection. Stringtrue/falsevalues inoptionsare converted toBooleanfor the provider.CopilotModelConnectionwithsource(DEPLOYMENT/LEGACY),priority,availableandunavailableReason;LegacyConfigProviderReadernow reportsLEGACY/ 100.CopilotConnectionValidator, run by the aggregator at load time and isolated per connection:providerIdandmodelare required; thelegacyconnection is usable only whenCOPILOT_API_KEYor the storedapiKeyprovides a non-blank key, so the provider's own environment fallback is never used;baseUrl, when set, must behttp(s)and resolve to a public address (loopback, link-local, site-local, multicast, ULA and100.100.100.200are rejected) unless its host is listed innacos.copilot.base-url.allowlist;canResolvemust acceptproviderId:model; and aresolveprobe must succeed. A failed connection stays in the table with its reason instead of disappearing.CopilotModelRouter/DefaultCopilotModelRouter:selectedConnection(only if enabled and available) > first available deployment connection > first available legacy connection >null. Connections are sorted by priority and then by name, so same-priority ties resolve deterministically rather than following property-source iteration order.CopilotDynamicConfig, aSubscriber<ServerConfigChangeEvent>registered withNotifyCenter, which refreshes the manager on a dedicated single-thread executor so the refresh does not run on the shared event publisher thread; the executor is shut down on destroy. It is modeled on core'sAbstractDynamicConfigbecause copilot must not depend on nacos-core, and the javadoc carries the TODO to merge them later.CopilotAgentManagernow routes through the router and no longer hard-codes a blank-key check: "an empty key means disabled" is enforced by the validator for the legacy connection only, so a deployment connection for a local model can omitapiKey. On refresh the manager also re-bindsnacos.copilotwithBinder, because the@ConfigurationPropertiesbean is bound once at startup andselectedConnection/enabled/studioUrlwould otherwise stay stale. AselectedConnectionthat is missing, disabled, or unavailable is reported with a warning at refresh time. AgentScope Studio is initialized once and re-initialized only afterstudioUrlorstudioProjectchanges and the initialization succeeds.Behaviour notes
legacyconnection is usable only with a resolved key and never falls back to the provider's environment variable, while a deployment connection may omit it (the local-model case from the issue body, e.g.ollama).nacos.copilot.base-url.allowlist(comma-separated hosts) bypasses the public-address check for explicitly trusted endpoints.watch()callback / diff-based refresh sketched in the 2026-10-15 comment is superseded: a refresh rebuilds the whole table, the manager resolves per request, and in-flight agents keep the client they started with.CopilotModelRouter/DefaultCopilotModelRouter(the earlier comment used the nameDefaultConnectionRouter).getConfigor the console yet; that is step 3.Verifying this change
mvn -B -pl copilot test: 174 tests, 0 failures, 0 errors, 0 skipped.mvn -B -pl copilot checkstyle:check spotless:check apache-rat:check spotbugs:check: BUILD SUCCESS, 0 checkstyle violations, 0 SpotBugs bugs, RAT approved 78 files.baseUrl/ allowlist / resolve-probe paths, routing order including the selected-but-unavailable fallback, refresh re-bindingselectedConnectionfrom the environment (with the rebind disabled the same test fails withexpected: <prod-qwen> but was: <null>), the ignored-selection warning for missing/disabled/unavailable connections, the dedicated refresh executor and its shutdown, and Studio re-initialization only on change.Part of #15906 (step 2 of 3), under #15757.