Skip to content

[ISSUE #15906] Load, validate and route Copilot model connections - #16017

Open
neoLsH wants to merge 5 commits into
alibaba:developfrom
neoLsH:develop-issue#15906-step2
Open

neoLsH wants to merge 5 commits into
alibaba:developfrom
neoLsH:develop-issue#15906-step2

Conversation

@neoLsH

@neoLsH neoLsH commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

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 legacy connection. This step adds the deployment-side connection table on top of it: connections declared under nacos.copilot.connections.* are read, merged with the legacy connection, 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

  • Add DeploymentConfigProviderReader (order 10): enumerates connection names from the enumerable property sources under nacos.copilot.connections. and binds each block into DeploymentConnectionDefinition separately, so one malformed block only drops that connection. String true/false values in options are converted to Boolean for the provider.
  • Extend CopilotModelConnection with source (DEPLOYMENT / LEGACY), priority, available and unavailableReason; LegacyConfigProviderReader now reports LEGACY / 100.
  • Add CopilotConnectionValidator, run by the aggregator at load time and isolated per connection: providerId and model are required; the legacy connection is usable only when COPILOT_API_KEY or the stored apiKey provides a non-blank key, so the provider's own environment fallback is never used; baseUrl, when set, must be http(s) and resolve to a public address (loopback, link-local, site-local, multicast, ULA and 100.100.100.200 are rejected) unless its host is listed in nacos.copilot.base-url.allowlist; canResolve must accept providerId:model; and a resolve probe must succeed. A failed connection stays in the table with its reason instead of disappearing.
  • Add 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.
  • Add CopilotDynamicConfig, a Subscriber<ServerConfigChangeEvent> registered with NotifyCenter, 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's AbstractDynamicConfig because copilot must not depend on nacos-core, and the javadoc carries the TODO to merge them later.
  • CopilotAgentManager now 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 omit apiKey. On refresh the manager also re-binds nacos.copilot with Binder, because the @ConfigurationProperties bean is bound once at startup and selectedConnection / enabled / studioUrl would otherwise stay stale. A selectedConnection that is missing, disabled, or unavailable is reported with a warning at refresh time. AgentScope Studio is initialized once and re-initialized only after studioUrl or studioProject changes and the initialization succeeds.
  • Update the refresh and connection rules of both integration-adapter specs (zh-CN and en).

Behaviour notes

  • The blank-key gate from step 1 is now source-specific: the legacy connection 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).
  • New deployment key: nacos.copilot.base-url.allowlist (comma-separated hosts) bypasses the public-address check for explicitly trusted endpoints.
  • The per-reader 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.
  • The router classes are CopilotModelRouter / DefaultCopilotModelRouter (the earlier comment used the name DefaultConnectionRouter).
  • Availability and rejection reasons are stored on each connection and logged, but not exposed through getConfig or 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.
  • New tests cover deployment block enumeration and per-block isolation, name normalization and cross-reader precedence, same-priority ordering by name, the validator's required-field / baseUrl / allowlist / resolve-probe paths, routing order including the selected-but-unavailable fallback, refresh re-binding selectedConnection from the environment (with the rebind disabled the same test fails with expected: <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.

- 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.
@github-actions

Copy link
Copy Markdown

Thanks for your this PR. 🙏
Please check again for your PR changes whether contains any usage/api/configuration change such as Add new API , Add new configuration, Change default value of configuration.
If so, please add or update documents(markdown type) in docs/next/ for repository nacos-group/nacos-group.github.io


感谢您提交的PR。 🙏
请再次查看您的PR内容,确认是否包含任何使用方式/API/配置参数的变更,如:新增API、新增配置参数、修改默认配置等操作。
如果是,请确保在提交之前,在仓库nacos-group/nacos-group.github.io中的docs/next/目录下添加或更新文档(markdown格式)。

…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.
@KomachiSion

Copy link
Copy Markdown
Collaborator

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 a12fd3f, the two layers disagree:

  • DeploymentConfigProviderReader passes the original connection name directly to Spring Binder. With otherwise identical valid provider/model/key fields, prod-qwen loads, while Prod-Qwen and PROD-QWEN are rejected with Configuration property name ... is not valid and dropped.
  • The connection table and aggregation logic normalize names with trim() and lowercasing, so lookup and conflict handling are case-insensitive.

The issue discussion currently describes case-insensitive names. Please make the intended contract explicit, including allowed characters and leading/trailing whitespace handling:

  • If names are case-insensitive, configuration parsing/binding must accept and normalize the permitted casing variants consistently before aggregation. Equivalent names must have consistent duplicate/override behavior, and selectedConnection must follow the same rule.
  • If names are case-sensitive, preserve case throughout parsing, duplicate/override handling, table lookup, and routing. The lowercasing introduced in step 1 must then be revisited, and the spec/design discussion updated accordingly.

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 Prod-Qwen versus prod-qwen that exercise the reader-to-aggregator-to-router path, including duplicate handling and selection with a different casing. The expected result should follow the agreed spec, rather than differ between layers.

…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.
Comment on lines +101 to +103
if (studioUrl.equals(activeStudioUrl) && studioProject.equals(activeStudioProject)) {
LOGGER.debug("AgentScope Studio is already initialized for {}", studioUrl);
return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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):

  1. Configure Studio URL A and refresh; initialization succeeds, so activeStudioUrl is cached as A.
  2. 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.
  3. Restore URL A and refresh. This equality check matches the stale cache and returns before calling initialize(), even though StudioManager.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.

Comment on lines +253 to +254
BindResult<CopilotProperties> bound = Binder.get(environment)
.bind(CONFIG_PREFIX, Bindable.of(CopilotProperties.class));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. Start from an upgraded installation where Nacos Config contains a valid copilot-config.json in group nacos-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 contain selectedConnection.
  2. Configure two enabled, available deployment connections, alpha and beta, under nacos.copilot.connections.*, and set nacos.copilot.selectedConnection=beta in the deployment configuration.
  3. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants