# Code Review 修复计划

**日期**: 2026-04-11  
**来源**: 三份 code review 报告提炼合并  
**范围**: 7 项修复 + 1 项风险提示（不纳入本次修复）

---

## 1. `secrets-core` 导入/导出链路

**目标**: 修复"导入丢失 secret type"。

**涉及文件**:
- `crates/secrets-core/src/models.rs`
- `crates/secrets-core/src/service/export.rs`
- `crates/secrets-core/src/service/import.rs`

**实施项**:
1. 在 `ExportEntry` 增加可选字段 `secret_types: Option<BTreeMap<String, String>>`，键为 secret 名，值为 type。
2. 修改导出逻辑，除了导出 `secrets` 的值，也把每个 secret 的 `type` 一并导出。
3. 修改导入逻辑，把 `entry.secret_types` 传给 `AddParams.secret_types`，不再用 `&Default::default()`。
4. 明确兼容旧导出文件：`secret_types` 缺失时继续默认 `"text"`。

**验证**:
1. 新增 round-trip 测试：带 `password` / `key` / `text` 三种类型的导出再导入，类型保持不变。
2. 新增向后兼容测试：旧格式导入时仍成功，默认回落到 `"text"`。

---

## 2. `secrets-core` env map

**目标**: 修复环境变量名碰撞。

**涉及文件**:
- `crates/secrets-core/src/service/env_map.rs`

**实施项**:
1. 统一分隔符策略：把字段名里的 `.` 替换成 `__`（双下划线），保留原始 `_` 为单下划线，避免 `db.password` 和 `db_password` 碰撞。
2. 如仍发生碰撞，显式返回错误，而不是 `HashMap::insert` 静默覆盖。

**验证**:
1. 添加测试覆盖：
   - `db.password` vs `db_password`
   - 带 `prefix` 的情况
   - 多 entry 合并时碰撞
2. 确认输出变量名文档与实现一致。

---

## 3. `secrets-core` rollback

**目标**: 修复回滚路径中的 TOCTOU 和"使用旧 live 值"问题。

**涉及文件**:
- `crates/secrets-core/src/service/rollback.rs`

**实施项**:
1. 把首次读取 live entry 的逻辑移入事务内，并与 `FOR UPDATE` 合并，避免在加锁前基于过期数据做决策。
2. 在拿到加锁后的 live row 后，再决定错误消息、审计字段和更新语句输入。
3. 明确回滚语义：
   - 若产品希望"完全回到快照"，则 `name/notes` 也从快照恢复。
   - 若希望保留当前标识，则代码和文档都要显式说明此设计决策。
4. 避免混用事务前的 `live_entry` 与事务内的 `lr`。

**验证**:
1. 新增测试覆盖：
   - 回滚时恢复字段是否符合预期
   - 并发更新 + 回滚场景不再依赖过期值

---

## 4. `secrets-core` API key

**目标**: 修复 `regenerate_api_key` 返回未持久化 key。

**涉及文件**:
- `crates/secrets-core/src/service/api_key.rs`

**实施项**:
1. 检查 `UPDATE` 的 `rows_affected()`。
2. 若为 `0`，返回 `NotFoundUser` 或等价业务错误。
3. 可选：改成 `UPDATE ... RETURNING api_key`，减少"先生成后判断"的分支。

**验证**:
1. 新增测试：
   - 正常用户可返回新 key
   - 不存在用户时返回错误，而不是返回伪成功 key

---

## 5. `secrets-mcp` tools

**目标**: 修复 MCP 层三个问题：
- `secrets_add` 提交后再回查 entry 的竞态
- `secrets_find` 计数失败被吞成 0
- `secrets_rollback` 冗余要求 encryption key

**涉及文件**:
- `crates/secrets-mcp/src/tools.rs`
- 可能连带 `crates/secrets-core/src/service/add.rs` 的返回结构

**实施项**:
1. 让 `svc_add` 直接返回 `entry_id`，MCP 不再在提交后用 `name+folder+user_id` 二次 `resolve_entry`。
2. `secrets_find` 中移除 `.unwrap_or(0)`：
   - 要么把计数错误向上传播
   - 要么返回 `total_count: null` / `count_unavailable: true`
3. `secrets_rollback` 改为只要求用户身份，不要求 encryption key。
4. 同步修正工具描述文案，避免 schema 与实际行为不一致。

**验证**:
1. `secrets_add`：父子关系新增时直接使用返回的 `entry_id`。
2. `secrets_find`：模拟 count 失败时，结果不再伪装成 `0`。
3. `secrets_rollback`：无 key 时可执行，工具描述与行为一致。

---

## 6. `secrets-mcp` Web 会话校验

**目标**: 让 JSON API 与页面路由的 `key_version` 校验对齐。

**涉及文件**:
- `crates/secrets-mcp/src/web/mod.rs`
- `crates/secrets-mcp/src/web/entries.rs`
- `crates/secrets-mcp/src/web/account.rs`
- 其他仅使用 `current_user_id()` 的 JSON handler

**实施项**:
1. 抽一个适用于 JSON API 的用户校验辅助函数。
2. 该函数应同时完成：
   - session 取 `user_id`
   - 加载用户
   - 比对 `SESSION_KEY_VERSION` 与 DB `key_version`
3. 页面路由继续保留 `require_valid_user`，JSON 路由统一改用等价校验。
4. 统一失败语义：
   - HTML 路由：重定向 `/login`
   - JSON 路由：返回 `401`

**验证**:
1. 新增测试覆盖：
   - `key_version` 一致时 JSON API 正常
   - `key_version` 不一致时 JSON API 返回 `401`
   - 用户删除/会话损坏时返回正确错误

---

## 7. Web API 输入校验

**目标**: 补齐 Web JSON API 的长度校验，避免 DB 约束错误落成 500。

**涉及文件**:
- `crates/secrets-mcp/src/web/entries.rs`
- 如有需要，抽公共 helper 到 `web/mod.rs` 或单独模块

**实施项**:
1. 在 `api_entry_patch` 对齐 MCP 的长度规则：
   - `folder <= 128`
   - `type <= 64`
   - `name <= 256`
   - `notes <= 10000`
2. 视情况复用 `validation` 常量，避免 Web/MCP 两套上限漂移。
3. 保持错误返回为 `400`，而不是依赖数据库报错。

**验证**:
1. 为超长 `folder/type/name/notes` 分别补测试。
2. 确认错误文案和现有本地化风格一致。

---

## 8. 暂不纳入本次修复（风险提示）

- 调试日志记录加密密钥片段（`extract_enc_key_or_arg` 在 debug 级别记录 key 前后缀）
- `encryption_key` 作为工具参数带来的日志/对话暴露面

**处理方式**: 记录为"接口安全风险提示"，待后续单独决定是否收紧 debug 日志、调整工具描述或限制参数传 key 的路径。

---

## 实施顺序

1. `secrets-core` 导入/导出链路
2. `secrets-core` env map
3. `secrets-core` rollback
4. `secrets-core` API key
5. `secrets-mcp` tools
6. `secrets-mcp` Web 会话校验
7. `secrets-mcp` Web 输入校验

**原因**: 先修 core 语义和返回结构，再修上层接入。`svc_add` 返回结构、rollback 语义、export/import 格式都属于底层契约，适合先稳定。

---

## 验证与收尾

1. 先跑相关单元/集成测试。
2. 再跑全量：
   ```bash
   cargo fmt -- --check
   cargo clippy --locked -- -D warnings
   cargo test --locked
   ```
3. 若涉及 `crates/**` 的实际改动，按 `AGENTS.md` 约定检查版本/tag，并执行 `./scripts/release-check.sh`。