pref(provider):完成OpenAI-compatible子包拆离#219
Conversation
|
/review -codex |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/review -codex |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/review -codex |
| var file persistedConfig | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
This changes the top-level config loader from the previous "read old keys, then rewrite to the canonical format" behavior into a hard startup failure for any existing config.yaml that still contains legacy keys like workdir, default_workdir, or providers. The updated tests explicitly codify that regression, so users upgrading from the prior build will now have to hand-edit their config before NeoCode can start. If the breaking change is intentional, it needs a migration path or at least a compatibility shim here rather than failing on load.
| var file customProviderFile | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
KnownFields(true) combined with removing openai_compatible.profile makes previously accepted custom provider configs fail to load immediately. The old tests covered profile, and this PR flips that to an error, so existing providers/*/provider.yaml files can now break startup after upgrade even though the field was just being ignored before. This needs either a backward-compatibility allowance for retired fields or an explicit migration step.
| @@ -22,15 +22,13 @@ type Loader struct { | |||
| } | |||
|
|
|||
| type persistedConfig struct { | |||
There was a problem hiding this comment.
Dropping workdir from persistedConfig removes the only YAML-backed way to configure the default workspace, but Config.Workdir is still validated and then fed into bootstrap, session storage, and tool sandbox setup. After this change a user can no longer persist a non-default workdir in ~/.neocode/config.yaml: old files fail to load, and newly saved files silently omit the setting. That is a behavior regression, not just a cleanup.
| var file persistedConfig | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
KnownFields(true) is now applied after the legacy keys were removed from the persisted schema, so existing installs with providers, provider_overrides, workdir, or default_workdir now hard-fail during startup instead of being rewritten into the current minimal format. The previous loader explicitly preserved that upgrade path, and the docs still say those old keys are cleaned up automatically, so this is a breaking migration regression.
| var file persistedConfig | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
KnownFields(true) 把主配置解析从“忽略/重写旧字段”改成了“遇到未知字段直接启动失败”。这会让已经存在的 ~/.neocode/config.yaml 里只要还保留 workdir、default_workdir、providers、workspace_root 等历史字段，就在升级后无法启动，而这次 PR 的目标只是 provider 目录重构。这里需要保留迁移/重写路径，或者显式做 config version bump，否则是一次无预警的兼容性回归。
| if normalizeProviderDriver(p.Driver) == "openai" { | ||
| return fmt.Errorf("provider %q driver %q is no longer supported", p.Name, p.Driver) | ||
| switch normalizedDriver { | ||
| case provider.DriverOpenAICompat, provider.DriverGemini, provider.DriverAnthropic: |
There was a problem hiding this comment.
这里把允许的 driver 固定成 3 个字符串后，ProviderConfig.Validate() 就不再与 provider.Registry 的扩展点一致了。现在即使运行时已经注册了一个新 driver，配置层也会先报 driver ... is not supported，导致新增 in-tree/第三方 driver 根本无法落地。更稳妥的做法是让配置校验依赖实际注册表/支持集，而不是在 config 包里再维护一份硬编码 allowlist。
| var file persistedConfig | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
Switching the root config parser to KnownFields(true) turns previously migratable configs into hard startup failures. On main, providers / provider_overrides were accepted and rewritten away during Load(), and the docs still describe that upgrade path. With this change, users carrying an older config.yaml can no longer boot to let the loader normalize it. Please preserve the legacy-key migration path (or rewrite before strict validation) instead of rejecting those files outright.
| var file customProviderFile | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
This strict decode also breaks existing custom-provider files that still include openai_compatible.profile. Before this PR, that field was tolerated as a no-op; after removing it from customOpenAICompatibleFile, upgrades now fail during startup with field profile not found. Please keep profile as a deprecated accepted field, or add a migration path, so previously valid provider directories continue to load.
| var file persistedConfig | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
Switching the root config parser to KnownFields(true) removes the existing upgrade path for legacy config.yaml keys. On main, providers / provider_overrides were tolerated and rewritten away during Load(), and the docs still describe that cleanup behavior. After this change, older configs become hard startup failures instead of being normalized.
| var file customProviderFile | ||
| if err := yaml.Unmarshal(data, &file); err != nil { | ||
| decoder := yaml.NewDecoder(bytes.NewReader(data)) | ||
| decoder.KnownFields(true) |
There was a problem hiding this comment.
This strict decode also breaks previously accepted custom-provider files that still carry retired fields such as openai_compatible.profile. Before this PR that field was a no-op; now startup fails with field profile not found. If the field is being retired, this needs a compatibility shim or migration path rather than an immediate load-time break.
| if normalizeProviderDriver(p.Driver) == "openai" { | ||
| return fmt.Errorf("provider %q driver %q is no longer supported", p.Name, p.Driver) | ||
| switch normalizedDriver { | ||
| case provider.DriverOpenAICompat, provider.DriverGemini, provider.DriverAnthropic: |
There was a problem hiding this comment.
ProviderConfig.Validate() now hardcodes the allowed driver names in config, which breaks the registry-based extension point. A driver can be successfully registered in provider.Registry and still be rejected during config load here with driver ... is not supported. This support check should come from the actual registry/support set instead of a second allowlist in config.
1 participant
变更说明
本 PR 基于当前工作树相对上次提交的实际变更，对 OpenAI-compatible provider 的目录结构与职责边界进行收敛。当前仓库虽然已经完成了
openai -> openaicompat的命名收口，但openaicompat根包仍然同时承载共享入口职责与/chat/completions具体协议实现，后续若继续接入responses，根包会再次膨胀成新的“大而全兼容桶”。这次改动的重点不是功能扩张，而是把已经可复用的 Chat Completions 协议处理逻辑迁移到独立子包，使
openaicompat根包回到“共享入口 + discovery + transport +api_style分流”的定位，并为后续responses子协议接入预留清晰扩展点。当前api_style: responses仍保持显式未支持状态，避免配置层已允许该值而 provider 层默默走错协议。主要改动
internal/provider/openaicompat/chatcompletions子包，集中承载/chat/completions协议实现。openaicompat根包中的请求结构、响应结构、请求组装、SSE 有界读取、流式事件转换、tool-call 增量拼装、usage 提取与 HTTP 错误解析迁移到子包。internal/provider/openaicompat/provider.go，使根包只负责：/modelsdiscoveryapi_style规范化与分流api_style增加显式分流行为：chat_completions走chatcompletions子包实现responses返回明确的未支持错误api_style直接失败internal/provider/openaicompat相关测试，改为覆盖新子包暴露的边界能力，并补充responses当前未支持的回归验证。openaicompat根包与chatcompletions子包的新边界。预期收益
openaicompat的共享职责与子协议职责边界更清晰，避免根包继续膨胀。internal/provider/openaicompat/responses的接入预留稳定落点，降低 Phase F 改造成本。if apiStyle == ...条件分支带来的协议耦合。/chat/completions主链可用的同时，让responses以显式失败的方式暴露未完成状态，降低排查成本。