168 lines
6.9 KiB
Markdown
168 lines
6.9 KiB
Markdown
# Deep Code Review: ocswitch
|
||
|
||
Date: 2026-04-17
|
||
Reviewer: Apale
|
||
Scope: `internal/config`, `internal/proxy`, `internal/opencode`, `internal/cli`, `README.md`
|
||
Verification: `go test ./...`, `go test -race ./...`
|
||
|
||
## Summary
|
||
|
||
本次 review 未发现立即可触发的数据破坏级致命 bug，但确认存在 2 个高风险项、4 个中风险项，以及 4 个低风险/改进项。高风险集中在配置文件并发写入和代理入站 body 无读取超时；中风险集中在流式连接悬挂、错误语义丢失、OpenCode 默认模型静默写坏，以及默认本地 API Key 在非 loopback 场景下的暴露风险。
|
||
|
||
## High Risk
|
||
|
||
### 1. Config/OpenCode save path is not truly safe under concurrent writers
|
||
|
||
- Files:
|
||
- `internal/config/config.go:147-175`
|
||
- `internal/opencode/opencode.go:84-97`
|
||
- Problem:
|
||
- 两处保存逻辑都使用固定临时文件名 `path + ".tmp"`，且没有文件锁。
|
||
- `opencode.Save` 还属于 read-modify-write，基于旧文件内容生成新内容，存在并发 stale read 覆盖问题。
|
||
- Trigger:
|
||
- 两个 CLI 进程、两个自动化脚本或未来后台流程同时写同一配置文件。
|
||
- Impact:
|
||
- 临时文件互相覆盖、最后写入者静默丢掉前一个更新、配置内容回退或错乱。
|
||
- Recommended fix:
|
||
- 抽公共原子写 helper。
|
||
- 使用 `os.CreateTemp(dir, base+".*.tmp")` 创建唯一 tmp。
|
||
- 写入后 `Sync` 文件、`Close`、`Rename`，然后 `Sync` 父目录。
|
||
- 外层增加 advisory file lock。
|
||
- `opencode.Save` 必须在锁内完成 read-modify-write，避免旧快照覆盖新文件。
|
||
|
||
### 2. Proxy request body has no read timeout after headers
|
||
|
||
- Files:
|
||
- `internal/proxy/server.go:79-83`
|
||
- `internal/proxy/server.go:148-151`
|
||
- Problem:
|
||
- 仅配置 `ReadHeaderTimeout`，body 通过 `io.ReadAll(http.MaxBytesReader(...))` 全量读取，没有 body read timeout。
|
||
- Trigger:
|
||
- 客户端快速发完 headers，然后慢速发送 body 或中途长期停住。
|
||
- Impact:
|
||
- slowloris 式连接占用，长期占住 goroutine/socket，代理容易被低成本拖死。
|
||
- Recommended fix:
|
||
- 给 `http.Server` 增加 `ReadTimeout`，至少覆盖 header+body 接收阶段。
|
||
- 若后续 body 可能变大，再把该值做成配置项。
|
||
- 将底层超时统一映射成稳定的客户端错误，而不是直接回传底层 I/O 文本。
|
||
|
||
## Medium Risk
|
||
|
||
### 3. Streaming response has no idle timeout once bytes start flowing
|
||
|
||
- Files:
|
||
- `internal/proxy/server.go:61-64`
|
||
- `internal/proxy/server.go:307-323`
|
||
- Problem:
|
||
- `http.Client.Timeout = 0`，流式转发阶段没有任何后续 idle timeout。
|
||
- Trigger:
|
||
- 上游先返回部分 chunk，随后长时间不再发送数据也不主动断开。
|
||
- Impact:
|
||
- 请求长期挂起，占住上下游连接与 handler，累计后形成资源耗尽。
|
||
- Recommended fix:
|
||
- 非流式请求使用整体超时。
|
||
- 流式请求增加“空闲超时”而非“总超时”，超时后取消上游 context 并中断转发。
|
||
|
||
### 4. Retryable upstream failures are collapsed into a generic 502
|
||
|
||
- Files:
|
||
- `internal/proxy/server.go:216-217`
|
||
- `internal/proxy/server.go:257-260`
|
||
- Problem:
|
||
- 上游 `429`/`5xx` 被当作可重试失败吞掉；所有 target 都失败时统一返回 `502 all upstream targets failed`。
|
||
- Trigger:
|
||
- 单个 target 返回 `429/5xx`，或所有 target 都返回 retryable failure。
|
||
- Impact:
|
||
- 客户端无法拿到真实 `429`、`Retry-After`、上游错误体；限流与退避语义丢失，排障信息也被抹平。
|
||
- Recommended fix:
|
||
- `tryOnce` 返回结构化失败信息（status/headers/body）。
|
||
- 失败链耗尽且尚未向下游写字节时，优先透传最后一个上游错误，至少保留 `429`、`Retry-After`、`Content-Type` 和裁剪后的 body。
|
||
|
||
### 5. `opencode sync --set-model` can silently write an invalid default model
|
||
|
||
- Files:
|
||
- `internal/cli/opencode.go:82-93`
|
||
- Problem:
|
||
- `--set-model` 和 `--set-small-model` 不校验输入是否是当前可路由 alias。
|
||
- Trigger:
|
||
- 用户输入不存在的 alias、已禁用 alias 或不符合 `ocswitch/<alias>` 约定的值。
|
||
- Impact:
|
||
- `sync` 表面成功，但会把 OpenCode 顶层默认模型写成无效值，形成静默坏配置。
|
||
- Recommended fix:
|
||
- 仅接受 `ocswitch/<alias>` 形式。
|
||
- `<alias>` 必须存在于 `cfg.AvailableAliasNames()`。
|
||
- 报错时给出候选 alias 列表。
|
||
|
||
### 6. Fixed default local API key remains unsafe when binding to non-loopback addresses
|
||
|
||
- Files:
|
||
- `internal/config/config.go:18-20`
|
||
- `internal/config/config.go:83-89`
|
||
- `internal/config/config.go:392-395`
|
||
- `internal/proxy/server.go:120-133`
|
||
- Problem:
|
||
- 默认本地 API Key 为固定公开值 `ocswitch-local`，同时校验逻辑未阻止用户在非 loopback 监听时继续使用默认 key。
|
||
- Trigger:
|
||
- 用户将 `server.host` 设置为 `0.0.0.0`、局域网 IP、容器映射地址等。
|
||
- Impact:
|
||
- 网络上任何知道默认 key 的访问者都可直接调用代理并消耗上游额度。
|
||
- Recommended fix:
|
||
- 在 `Validate()` 中增加约束：非 loopback 监听时拒绝默认 key。
|
||
- 更稳妥方案是首次生成随机 key，只在纯本机模式允许默认值。
|
||
|
||
## Low Risk / Improvement Options
|
||
|
||
### 7. Alias lifecycle is incomplete in CLI
|
||
|
||
- File: `internal/cli/alias.go:35-84`
|
||
- Problem:
|
||
- 缺少 `alias enable/disable`，alias 一旦禁用后没有 CLI 恢复路径。
|
||
- Options:
|
||
- 增加 `alias enable` / `alias disable` 子命令。
|
||
- 将 `alias add` 改成三态更新：未指定、启用、禁用。
|
||
|
||
### 8. Import behavior and docs are inconsistent for empty API key
|
||
|
||
- Files:
|
||
- `internal/opencode/opencode.go:581-594`
|
||
- `internal/cli/provider.go:273-275`
|
||
- Problem:
|
||
- 实现允许导入只有 `baseURL`、没有 `apiKey` 的 provider，但帮助文本/README 表述为需要两者同时存在。
|
||
- Options:
|
||
- 收紧实现，要求 `apiKey != ""`。
|
||
- 或放宽文档，明确允许导入空 key provider。
|
||
|
||
### 9. `opencode sync` still has broader side effects than users may expect
|
||
|
||
- Files:
|
||
- `internal/opencode/opencode.go:58-60`
|
||
- `internal/opencode/opencode.go:80-83`
|
||
- `internal/opencode/opencode.go:424-428`
|
||
- Problem:
|
||
- `opencode sync` 会丢失 JSONC 注释，还可能补写 `$schema`。
|
||
- Options:
|
||
- 在 CLI help/README 中明确副作用。
|
||
- 长期方案是引入更细粒度的 JSONC patch 机制，尽量避免无关键重写。
|
||
|
||
### 10. Header forwarding is still broader than ideal
|
||
|
||
- Files:
|
||
- `internal/proxy/server.go:394-440`
|
||
- Problem:
|
||
- 目前只做固定 hop-by-hop 过滤，转发策略仍偏黑名单。
|
||
- Options:
|
||
- 解析 `Connection` 动态声明的 hop-by-hop headers 并移除。
|
||
- 逐步收敛成最小白名单转发。
|
||
|
||
## Testing Notes
|
||
|
||
- `go test ./...` passed
|
||
- `go test -race ./...` passed
|
||
|
||
## Coverage Gaps Observed
|
||
|
||
- 未覆盖并发写配置文件的场景
|
||
- 未覆盖慢速 body 读取超时场景
|
||
- 未覆盖全部上游 `429/5xx` 时的错误透传语义
|
||
- 未覆盖 `--set-model` / `--set-small-model` 非法值校验
|