Skip to content

fix: wire persisted provider configuration into runtime - #3299

Open
DaBear987 wants to merge 1 commit into
ultraworkers:mainfrom
DaBear987:main
Open

fix: wire persisted provider configuration into runtime#3299
DaBear987 wants to merge 1 commit into
ultraworkers:mainfrom
DaBear987:main

Conversation

@DaBear987

Copy link
Copy Markdown

Wires persisted provider configuration from runtime settings into actual provider execution. Provider kind is authoritative when explicitly configured, with regression coverage for Anthropic, xAI, OpenAI, DashScope, and local OpenAI-compatible routing. Validation completed with cargo fmt --check, runtime/API/CLI test suites, and git diff --check.

@1716775457damn

Copy link
Copy Markdown

Good change — making provider kind authoritative removes the ambiguity of a persisted config silently overriding the routing. One question for clarity: with kind authoritative, what wins against an explicit runtime hint, e.g. a --provider flag or a model prefix on the command line? If the stored kind always wins regardless of the current invocation, a user doing a one-off provider switch could be surprised. Either a short precedence note in the docs or a test covering flag-vs-persisted precedence would make the intent unambiguous.

@1716775457damn

Copy link
Copy Markdown

补充建议形成闭环:优先级建议定为「显式运行期参数(--provider / 命令行 model 前缀)> 持久化配置中的 provider kind > 默认推断路由」。这样持久化配置保证会话间一致,命令行参数保留单次覆盖能力,符合多数 CLI 惯例。若同意此语义,可补一条 flag-vs-persisted 的优先级回归测试;runtime 测试套件这边需要的话我可以协助补。

@1716775457damn

Copy link
Copy Markdown

5adb8be (fix: honor explicit model provider over persisted config) 正好落实了我上次建议的优先级语义——显式运行期参数优先于持久化 provider kind,符合 CLI 惯例,赞。合入前一个小建议:可以合并掉那批临时的 CI patcher / stage / export commits(82eae82~29c904a),用一个干净的功能 commit 承载 5adb8be 的改动,评审与 bisect 都会更清爽。

@1716775457damn

Copy link
Copy Markdown

Re-checked at 5adb8be — the "explicit args > persisted kind > default inference" priority is now correctly applied through the injection path, matching what we discussed; the fallback chain reads clean. Also noticed the temporary PR3299 CI hook has been reverted (530a37f), good cleanup so it won't pollute the final merge. Worth re-running the full CI once more on the cleaned head before merge to confirm nothing regressed.

@1716775457damn

Copy link
Copy Markdown

Reviewed the force-push to f8ad763 — the head stays consistent with the "explicit args > persisted kind > default inference" semantics we settled on, and the temp CI hook is out of the final diff. No new concerns; please run the full CI once on this cleaned head before merge and we can get this in.

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