codexreview

.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/<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` 非法值校验

.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 ./...`

.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 ./..."
+    ]
+  }
+}

.trellis/workspace/Apale/index.md

@@ -8,7 +8,7 @@
 
 <!-- @@@auto:current-status -->
 - **Active File**: `journal-1.md`
-- **Total Sessions**: 6
+- **Total Sessions**: 7
 - **Last Active**: 2026-04-17
 <!-- @@@/auto:current-status -->
 
@@ -19,7 +19,7 @@
 <!-- @@@auto:active-documents -->
 | File | Lines | Status |
 |------|-------|--------|
-| `journal-1.md` | ~249 | Active |
+| `journal-1.md` | ~297 | Active |
 <!-- @@@/auto:active-documents -->
 
 ---
@@ -29,6 +29,7 @@
 <!-- @@@auto:session-history -->
 | # | 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` |

.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