From 51f030fb8ccba55b4d2cf243350af88003cc7446 Mon Sep 17 00:00:00 2001 From: apale7 <1092377056@qq.com> Date: Fri, 17 Apr 2026 22:27:49 +0800 Subject: [PATCH] codexreview --- .../04-17-deep-code-review-ocswitch/review.md | 167 ++++++++++++++++++ .../session-content.md | 17 ++ .../04-17-deep-code-review-ocswitch/task.json | 71 ++++++++ .trellis/workspace/Apale/index.md | 5 +- .trellis/workspace/Apale/journal-1.md | 48 +++++ 5 files changed, 306 insertions(+), 2 deletions(-) create mode 100644 .trellis/tasks/04-17-deep-code-review-ocswitch/review.md create mode 100644 .trellis/tasks/04-17-deep-code-review-ocswitch/session-content.md create mode 100644 .trellis/tasks/04-17-deep-code-review-ocswitch/task.json diff --git a/.trellis/tasks/04-17-deep-code-review-ocswitch/review.md b/.trellis/tasks/04-17-deep-code-review-ocswitch/review.md new file mode 100644 index 0000000..73185bf --- /dev/null +++ b/.trellis/tasks/04-17-deep-code-review-ocswitch/review.md @@ -0,0 +1,167 @@ +# 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/` 约定的值。 +- Impact: + - `sync` 表面成功，但会把 OpenCode 顶层默认模型写成无效值，形成静默坏配置。 +- Recommended fix: + - 仅接受 `ocswitch/` 形式。 + - `` 必须存在于 `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` 非法值校验 diff --git a/.trellis/tasks/04-17-deep-code-review-ocswitch/session-content.md b/.trellis/tasks/04-17-deep-code-review-ocswitch/session-content.md new file mode 100644 index 0000000..7aaaffd --- /dev/null +++ b/.trellis/tasks/04-17-deep-code-review-ocswitch/session-content.md @@ -0,0 +1,17 @@ +- Added `.trellis/tasks/04-17-deep-code-review-ocswitch/review.md` to capture the full deep review output and concrete remediation guidance. +- Recorded 2 high-risk items: + - concurrent config/OpenCode save paths are unsafe under concurrent writers + - proxy request body has no read timeout after headers +- Recorded 4 medium-risk items: + - streaming responses have no idle timeout + - retryable upstream failures are collapsed into a generic `502` + - `opencode sync --set-model` / `--set-small-model` can silently write invalid defaults + - fixed default API key remains unsafe when binding to non-loopback addresses +- Recorded 4 low-risk or improvement items: + - alias lifecycle lacks enable/disable recovery path + - provider import docs and implementation disagree on empty `apiKey` + - `opencode sync` side effects on JSONC comments and `$schema` need clearer docs + - header forwarding should better handle dynamic hop-by-hop headers and narrower forwarding rules +- Verification already completed during review: + - `go test ./...` + - `go test -race ./...` diff --git a/.trellis/tasks/04-17-deep-code-review-ocswitch/task.json b/.trellis/tasks/04-17-deep-code-review-ocswitch/task.json new file mode 100644 index 0000000..b01d729 --- /dev/null +++ b/.trellis/tasks/04-17-deep-code-review-ocswitch/task.json @@ -0,0 +1,71 @@ +{ + "id": "deep-code-review-ocswitch", + "name": "deep-code-review-ocswitch", + "title": "Deep code review for ocswitch", + "description": "Record deep code review findings, risks, and remediation options for the current ocswitch implementation.", + "status": "completed", + "dev_type": "docs", + "scope": "review", + "package": null, + "priority": "P1", + "creator": "Apale", + "assignee": "Apale", + "createdAt": "2026-04-17", + "completedAt": "2026-04-17", + "branch": null, + "base_branch": "master", + "worktree_path": null, + "current_phase": 0, + "next_action": [ + { + "phase": 1, + "action": "brainstorm" + }, + { + "phase": 2, + "action": "research" + }, + { + "phase": 3, + "action": "implement" + }, + { + "phase": 4, + "action": "check" + }, + { + "phase": 5, + "action": "update-spec" + }, + { + "phase": 6, + "action": "record-session" + } + ], + "commit": null, + "pr_url": null, + "subtasks": [], + "children": [], + "parent": null, + "relatedFiles": [ + ".trellis/tasks/04-17-deep-code-review-ocswitch/review.md", + "internal/config/config.go", + "internal/proxy/server.go", + "internal/opencode/opencode.go", + "internal/cli/opencode.go", + "internal/cli/alias.go", + "internal/cli/provider.go", + "README.md" + ], + "notes": "Completed a deep code review and recorded 2 high-risk, 4 medium-risk, and 4 low-risk/improvement findings with concrete remediation guidance.", + "meta": { + "reviewType": "deep-code-review", + "highRiskCount": 2, + "mediumRiskCount": 4, + "lowRiskCount": 4, + "verification": [ + "go test ./...", + "go test -race ./..." + ] + } +} diff --git a/.trellis/workspace/Apale/index.md b/.trellis/workspace/Apale/index.md index db68c8c..5ddf88c 100644 --- a/.trellis/workspace/Apale/index.md +++ b/.trellis/workspace/Apale/index.md @@ -8,7 +8,7 @@ - **Active File**: `journal-1.md` -- **Total Sessions**: 6 +- **Total Sessions**: 7 - **Last Active**: 2026-04-17 @@ -19,7 +19,7 @@ | File | Lines | Status | |------|-------|--------| -| `journal-1.md` | ~249 | Active | +| `journal-1.md` | ~297 | Active | --- @@ -29,6 +29,7 @@ | # | Date | Title | Commits | Branch | |---|------|-------|---------|--------| +| 7 | 2026-04-17 | Deep code review for ocswitch | - | `master` | | 6 | 2026-04-17 | Rename CLI to ocswitch | `cf3dcec` | `master` | | 5 | 2026-04-17 | Fix sync command panic | `887eb14` | `master` | | 4 | 2026-04-17 | Review MVP completion status | - | `master` | diff --git a/.trellis/workspace/Apale/journal-1.md b/.trellis/workspace/Apale/journal-1.md index d3ea964..c17b221 100644 --- a/.trellis/workspace/Apale/journal-1.md +++ b/.trellis/workspace/Apale/journal-1.md @@ -247,3 +247,51 @@ Renamed the CLI and synced docs, examples, and Trellis history from olpx/opswitc ### Next Steps - None - task complete + + +## Session 7: Deep code review for ocswitch + +**Date**: 2026-04-17 +**Task**: Deep code review for ocswitch +**Branch**: `master` + +### Summary + +Recorded the deep code review findings in Trellis, including 2 high-risk, 4 medium-risk, and 4 low-risk/improvement items with concrete remediation guidance. + +### Main Changes + +- Added `.trellis/tasks/04-17-deep-code-review-ocswitch/review.md` to capture the full deep review output and concrete remediation guidance. +- Recorded 2 high-risk items: + - concurrent config/OpenCode save paths are unsafe under concurrent writers + - proxy request body has no read timeout after headers +- Recorded 4 medium-risk items: + - streaming responses have no idle timeout + - retryable upstream failures are collapsed into a generic `502` + - `opencode sync --set-model` / `--set-small-model` can silently write invalid defaults + - fixed default API key remains unsafe when binding to non-loopback addresses +- Recorded 4 low-risk or improvement items: + - alias lifecycle lacks enable/disable recovery path + - provider import docs and implementation disagree on empty `apiKey` + - `opencode sync` side effects on JSONC comments and `$schema` need clearer docs + - header forwarding should better handle dynamic hop-by-hop headers and narrower forwarding rules +- Verification already completed during review: + - `go test ./...` + - `go test -race ./...` + + +### Git Commits + +(No commits - planning session) + +### Testing + +- [OK] (Add test results) + +### Status + +[OK] **Completed** + +### Next Steps + +- None - task complete