merge: code-review fixes (d7720662 baseline + 9f8a 文案常量化、env_prefix 测试、补充用例); secrets-mcp 0.5.21
- default 已 rebase 到 d7720662；合并说明见 plans/merge-code-review-fixes-2026-04-11.md - Web PATCH 长度错误用 validation 常量拼接；env_map 单测；import/api_key 单测 - rustfmt 收尾
This commit is contained in:
voson
143
plans/merge-code-review-fixes-2026-04-11.md
Normal file
143
plans/merge-code-review-fixes-2026-04-11.md
Normal file
@@ -0,0 +1,143 @@
|
||||
# Code Review 修复方案合并计划
|
||||
|
||||
**日期**: 2026-04-11
|
||||
**来源**: 两个 AI 实现对比评估
|
||||
**比较对象**:
|
||||
- `d7720662` (`/Users/voson/work/refining/secrets-cr-fixes-ws`)
|
||||
- `9f8a68cd` (`/Users/voson/work/refining/secrets/plan-impl`)
|
||||
|
||||
---
|
||||
|
||||
## 结论
|
||||
|
||||
以 **`d7720662`** 为主线采纳。
|
||||
|
||||
**原因**:
|
||||
1. `rollback` 的 live row 加锁与 snapshot 读取都在事务内完成，更符合原计划里对 TOCTOU 的修复要求。
|
||||
2. Web JSON API 的 session 校验保留了按 `UiLang` 返回错误信息的行为，没有把错误消息退化成固定英文。
|
||||
3. `svc_add` 返回 `entry_id`，MCP 层直接使用返回值建立 parent relation，和计划第 5 项更一致。
|
||||
|
||||
---
|
||||
|
||||
## 采纳策略
|
||||
|
||||
### 1. 主线保留 `d7720662`
|
||||
|
||||
保留以下实现，不从另一份实现回退：
|
||||
|
||||
- `crates/secrets-core/src/service/rollback.rs`
|
||||
- `crates/secrets-mcp/src/web/mod.rs`
|
||||
- `crates/secrets-core/src/service/add.rs`
|
||||
- `crates/secrets-mcp/src/tools.rs`
|
||||
|
||||
### 2. 从 `9f8a68cd` 手动吸收的小改动
|
||||
|
||||
仅吸收下面两处，手动改写，不直接整文件 cherry-pick：
|
||||
|
||||
1. `crates/secrets-mcp/src/web/entries.rs`
|
||||
- 把长度校验报错文案改成基于 `crate::validation::*` 常量拼接，避免上限数字硬编码在文案里。
|
||||
|
||||
2. `crates/secrets-core/src/service/env_map.rs`
|
||||
- 补 `env_prefix_with_and_without_prefix` 单测。
|
||||
|
||||
---
|
||||
|
||||
## 明确不采纳的实现
|
||||
|
||||
### 不采纳 `9f8a68cd` 的 `rollback.rs`
|
||||
|
||||
原因：
|
||||
- 它仍然先在事务外读取 `entries_history`，再开启事务并锁 live row。
|
||||
- 对“回滚到最近快照”的路径仍存在先读后锁的时间窗口。
|
||||
|
||||
### 不采纳 `9f8a68cd` 的 `web/mod.rs`
|
||||
|
||||
原因：
|
||||
- `load_session_user_strict()` / `require_valid_user_json()` 返回固定英文 JSON 错误。
|
||||
- 会丢失现有多语言错误语义。
|
||||
|
||||
### 不采纳 `9f8a68cd` 的 `AddResult.id`
|
||||
|
||||
原因：
|
||||
- 本轮计划里明确要求 `svc_add` 返回 `entry_id`。
|
||||
- `d7720662` 的字段命名与 MCP 使用方式更贴近计划要求。
|
||||
|
||||
---
|
||||
|
||||
## 与原计划的覆盖情况
|
||||
|
||||
两份实现都完成了大部分代码修改，但验证项整体没有补齐，当前更像“代码已改，测试仍不足”。
|
||||
|
||||
### 已基本完成
|
||||
|
||||
1. 导入/导出链路补 `secret_types`
|
||||
2. env map `.` -> `__`，并在冲突时返回错误
|
||||
3. API key `rows_affected()` 检查
|
||||
4. MCP `secrets_add` 避免提交后二次回查竞态
|
||||
5. MCP `secrets_find` 不再把 count 错误吞成 `0`
|
||||
6. MCP `secrets_rollback` 不再要求 encryption key
|
||||
7. Web JSON API 引入 `key_version` 严格校验
|
||||
8. Web PATCH 补长度校验
|
||||
|
||||
### 尚需补齐的验证
|
||||
|
||||
1. export/import round-trip 测试
|
||||
- `password` / `key` / `text` 三种类型导出再导入后保持不变
|
||||
|
||||
2. legacy import 测试
|
||||
- 老格式缺失 `secret_types` 时默认回落到 `text`
|
||||
|
||||
3. env map 测试
|
||||
- `db.password` vs `db_password`
|
||||
- 带 `prefix`
|
||||
- 多 entry 合并冲突
|
||||
|
||||
4. rollback 测试
|
||||
- 恢复字段是否符合预期
|
||||
- 并发更新 + 回滚不依赖过期值
|
||||
|
||||
5. `regenerate_api_key` 测试
|
||||
- 正常用户返回新 key
|
||||
- 不存在用户返回错误
|
||||
|
||||
6. MCP tool 测试
|
||||
- `secrets_find` count 失败路径
|
||||
- `secrets_rollback` 无 encryption key 也可执行
|
||||
|
||||
7. Web session / validation 测试
|
||||
- `key_version` mismatch -> `401`
|
||||
- 用户不存在 / session 损坏 -> 正确错误
|
||||
- `folder/type/name/notes` 超长 -> `400`
|
||||
|
||||
---
|
||||
|
||||
## 执行步骤
|
||||
|
||||
1. 以 `d7720662` 对应实现为合并基线。
|
||||
2. 手动吸收 `9f8a68cd` 中 `web/entries.rs` 的常量化长度报错文案。
|
||||
3. 手动吸收 `9f8a68cd` 中 `env_map.rs` 的 `env_prefix_with_and_without_prefix` 测试。
|
||||
4. 不引入 `9f8a68cd` 的 `rollback.rs`、`web/mod.rs`、`AddResult.id` 方案。
|
||||
5. 针对原计划缺失的验证项补测试。
|
||||
6. 跑质量门禁：
|
||||
|
||||
```bash
|
||||
cargo fmt -- --check
|
||||
cargo clippy --locked -- -D warnings
|
||||
cargo test --locked
|
||||
```
|
||||
|
||||
7. 跑发布前检查：
|
||||
|
||||
```bash
|
||||
./scripts/release-check.sh
|
||||
```
|
||||
|
||||
8. 确认版本和 tag：
|
||||
- `crates/secrets-mcp/Cargo.toml` 已 bump（合并执行时为 `0.5.21`，因 `crates/**` 有变更）
|
||||
- `jj tag list`
|
||||
|
||||
---
|
||||
|
||||
## 备注
|
||||
|
||||
如果后续要做最终合并，建议以 `d7720662` 为基础继续补测试，而不是尝试把两份实现整合成第三套逻辑。这样改动面最小，风险也最低。
|
||||
Reference in New Issue
Block a user