From a036ce9a8ee04bfdc1ccd8cb82c990b1cb3121cb Mon Sep 17 00:00:00 2001 From: ila Date: Thu, 16 Jul 2026 21:09:25 +0800 Subject: [PATCH] Add Phase 0 skeleton review with cross-check ruling Record the full-stack review of the Phase 0 skeleton (T-001~T-004), Codex's second-pass rebuttal, and the cross-checked final ruling: confirmed facts (with evidence), accept/correct notes, and a finalized action order. P1 (root go.work Win7 version bleed) is the only structural fix flagged for near-term handling. Co-Authored-By: Claude Fable 5 --- docs/review/phase0-skeleton-review.md | 227 ++++++++++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 docs/review/phase0-skeleton-review.md diff --git a/docs/review/phase0-skeleton-review.md b/docs/review/phase0-skeleton-review.md new file mode 100644 index 0000000..f2ffaee --- /dev/null +++ b/docs/review/phase0-skeleton-review.md @@ -0,0 +1,227 @@ +# Phase 0 工程骨架审查(T-001 ~ T-004) + +> 审查范围:提交 `cf9fc01`(T-001)、`0e20dd7`(T-002)、`6f920eb`(T-003)、`45c242e`(T-004)所落地的 monorepo 骨架、domain 状态模型、事件 runtime、Gio 空窗口、平台 stub 与验证闸门。 +> 审查视角:全栈开发工程师。仓库在审查时已推进到 T-301,本文只覆盖 Phase 0;安全关键模块(T-101~T-103、T-301)另行专项审查。 +> 日期:2026-07-16 + +## 总体判断 + +Phase 0 质量在平均线以上。最难的部分——**现代版 / Win7 双工具链隔离**——处理正确且稳健,这是最容易翻车的环节。存在几处值得优化的点,但没有返工级硬伤。M1 验收(双目标可编译、core 无头可测)成立。 + +## 做得好的地方 + +1. **分层边界是机器强制的,不是口头约定。** `scripts/check_core_boundaries.py` 用 `go list -json` 解析 core 的真实 import 图,拒绝 Gio / Windows / SQLite / app 层依赖,并自带 `--self-test` 防匹配器失效。core 确实零 UI、零 Windows 依赖。 + +2. **平台层 build-tag stub 模式正确。** `app-*/platform/windows/platform_windows.go`(`//go:build windows`)+ `platform_stub.go`(`//go:build !windows`),保证 core 与平台契约能在无头 Linux CI 跑测试。 + +3. **工具链隔离稳健。** 独立的 `app-win7/go.work`;`scripts/verify_phase0.sh` 对每个目标显式设置 `GOWORK=...` 而非依赖 cwd 自动发现;`GOTOOLCHAIN` 双钉(go1.25.0 / go1.20.14);`scripts/check_go_versions.py` 扫描 win7 构建列表里每个模块的 `go` 指令,拦截任何 >1.20 的依赖。这是防"依赖被隐式升级到要求 Go 1.21+"的正确做法。 + +4. **验证闸门可一键复现。** `verify_phase0.sh` 串起治理检查 + 边界检查 + 版本检查 + core vet/test + 双目标构建;`dist/` 中两个 EXE 均真实产出。 + +5. **事件 runtime 是生产级并发质量。** `core/application/runtime.go` 使用 `closeOnce` 防重复关闭、双 `select` 让 `done` 通道优先于入队、context 取消,非占位骨架。 + +6. **分发构建参数正确。** `CGO_ENABLED=0 -trimpath -ldflags="-H=windowsgui"`:单文件、无控制台窗口、可复现路径。 + +7. **测试有真实断言。** 状态迁移、事件 runtime(含 context 取消)、AppShell 布局尺寸、平台 stub 契约均有覆盖。 + +## 需优化项(按优先级) + +### P1 · `app-win7` 不应同时出现在根 `go.work` + +**现状:** 根 `go.work` 同时 `use ./app-modern`、`./app-win7`、`./core`,于是同一 workspace 混入 Gio v0.6.0 与 v0.10.1,MVS 统一为 v0.10.1。同时 `app-win7/go.work` 又单独存在(只含 win7 + core)。 + +**影响:** 不是致命问题——gate 通过、两个 EXE 都能出,因为构建按目标显式指定 `GOWORK`,win7 走自己的 workspace。但代价具体存在: + +- IDE / gopls 打开仓库根时,用 v0.10.1 类型检查 app-win7 代码,而其实际编译用 v0.6.0,会产生假报错或漏报真实的 v0.6.0 API 不兼容。 +- 从根目录跑 `go build ./...` 或 `go work sync` 时,app-win7 被拉进 v0.10.1 解析,是潜在雷。 + +**建议:** 根 `go.work` 只保留 `./app-modern` 与 `./core`,win7 完全交给 `app-win7/go.work`。这样没有任何一个 workspace 会混两个 Gio 版本,隔离彻底。两个 workspace 本是该项目不可避免的结构,问题只在于把 app-win7 重复塞进了根。 + +### P2 · 两个 `ui/gio/shell.go` 约 95% 重复 + +**现状:** `app-modern/ui/gio/shell.go` 与 `app-win7/ui/gio/shell.go` 的 `shellColors` 调色板、`NewTheme`、`Layout` 结构几乎一字不差。 + +**影响:** 一部分重复是固有的(core 不能 import Gio,两版 Gio API 不同);但调色板(纯数据)与布局骨架(结构)目前是硬拷,每新增一个视图重复就翻倍。 + +**建议:** 到 T-203+ 真正落地"共享 ViewModel / 设计 token + 隔离 Gio 适配"的纪律。已观察到 T-203 把列表状态放进 `core/application.CatalogListModel`,方向正确,需持续保持;纯数据的调色板值得抽到一处共享来源。 + +### P3 · 状态机迁移表偏宽松 + +**现状:** `core/domain/status.go` 的 `allowedTransitions` 允许 `Downloading→Installed`、`Queued→Installed`、`Failed→Installed` 等直接跳转,绕过 `verifying → extracting → installing` 序列。 + +**影响:** 状态机只是 UI 状态、非真正安装门禁,但对签名更新器,允许状态模型直接跳到 `installed` 会让"跳过校验的已安装"变得可表达,增加误表征风险。 + +**建议:** 若这些是"已最新、无操作"的快路径,加注释说明其语义;否则收紧迁移表,强制经过校验序列。 + +### P4 · CI 目前只有本地脚本 + +**现状:** `03-tech-stack.md` 记为"Gitea Actions 模板 + 远端 runner 可用性待确认",闸门只在本地跑。 + +**影响:** 对签名分发 / 自更新产品,只在本地跑的 gate 会腐化。Phase 0 验收写的是"CI 或本地脚本",算达标,但不应停留于此。 + +**建议:** 近期把 `verify_phase0` 真正挂到 push / PR。 + +### P5 · 杂项 + +- 未跟踪的 `soft_quay.code-workspace`(编辑器文件)应加入 `.gitignore`。 +- 确认 `go1.25.0` 这个 pin 在 CI 环境可获取(执行记录显示 win7 工具链是靠 `golang.org/dl/go1.20.14` 现场下载的)。 + +## 后续 + +真正的风险不在 Phase 0 骨架,而在已提交的 **T-101(清单验签)、T-102(ZIP 安全解压)、T-103(原子切换回滚)、T-301(下载队列)**——这些是"错了等于远程代码执行 / 装机损坏 / 数据丢失"的模块。从 `api.md` 与 `04-architecture.md` 的执行记录看实现相当细致,但正因如此需要逐行安全审计,不能只凭其自带测试通过判定。建议单独开一轮针对这四个模块的安全审查(实际读解压与验签代码、寻找绕过路径),另存 `docs/review/`。 + +## 全栈二次复核 + +> 复核视角:Codex 全栈开发工程师。 +> 复核方式:对照 Phase 0 原始提交、当前代码、workspace 实际模块解析结果和验证脚本逐条核验。 + +### 复核结论 + +原审核的总体判断成立:Phase 0 没有返工级硬伤,M1 验收有效。双工具链构建、core 无头测试、平台 build tag 和本地完整闸门都已实际工作。 + +但原审核约有三类内容需要校正: + +1. P1 是真实且优先级最高的工程结构问题。 +2. P2/P3 的现象存在,但不能直接按原建议实施,否则可能破坏现有双 Gio 隔离或合法的状态恢复语义。 +3. 原审核高估了 application Runtime 的关闭并发质量,并遗漏了边界检查范围与 vendor 文档不一致。 + +### 对原优化项的修订 + +#### P1 · 接受:彻底隔离根 workspace 与 Win7 workspace + +实际执行模块解析: + +- 根 `go.work` 将 `app-win7` 使用的 `gioui.org` 解析为 v0.10.1。 +- `app-win7/go.work` 将同一模块正确解析为 v0.6.0。 + +根 workspace 下的 win7 测试目前仍能通过,但这不能证明 v0.6.0 兼容,反而可能掩盖仅在旧 Gio API 下出现的问题。因此建议采纳原 P1,但实施必须同时完成: + +1. 根 `go.work` 只保留 `app-modern` 与 `core`。 +2. `app-win7` 继续只由 `app-win7/go.work` 管理。 +3. `init` / `verify_phase0` 对两个 workspace 分别执行 `go work sync`,不能移除 win7 后只同步根 workspace。 +4. 更新 `docs/03-tech-stack.md` 中“三模块根 workspace”的旧描述。 +5. 验证 gopls / IDE 打开仓库根时能正确识别嵌套 win7 workspace,避免从“版本污染”变成“win7 模块不在 workspace”。 + +该问题主要影响 IDE、根目录命令和兼容性假阴性。正式构建闸门已显式指定不同 `GOWORK`,因此当前发布构建仍是隔离的。 + +#### P2 · 部分接受:共享业务模型,不贸然共享 Gio 控件 + +Phase 0 两个 `shell.go` 的文本相似度约为 95.6%;推进到 T-204 后仍存在大量相似结构,维护成本判断正确。 + +但 T-003 明确要求 modern 与 win7 不共享 Gio 控件代码,而 Gio v0.6 与 v0.10 会继续产生 API 和布局能力差异。整改纪律应为: + +- 搜索、分类、选择、状态组合等无 UI 逻辑继续放在 `core/application` ViewModel。 +- modern/win7 的 Gio widget、Layout 和交互适配保持独立。 +- 不为了调色板把 `gioui.org`、UI 控件或平台表现概念引入 core。 +- 只有颜色/间距等纯数据出现明显漂移时,再评估生成式 design token 或跨 adapter 一致性测试;当前不值得为少量 token 新增共享模块。 + +因此 P2 是持续关注项,不是 Phase 0 必须立即重构的问题。 + +#### P3 · 修正风险解释:不要直接收紧恢复到 installed 的迁移 + +`Queued/Downloading/Failed → Installed|UpdateAvailable|NotInstalled` 可表示: + +- 已安装软件更新取消/失败后恢复原基础状态。 +- 活跃操作结束后重新按磁盘事实纠正 UI 状态。 +- 启动恢复时清除过期操作态。 + +当前 `ValidateTransition` / `CanTransition` 只在状态测试中使用,并未成为下载、验签或安装流程的安全门禁。因此“可直接跳过校验成为 installed”目前是模型语义不清,不是实际安全绕过。 + +建议: + +1. 为这些迁移补充“恢复基础状态”的注释和测试。 +2. T-302 安装编排必须独立强制 download → verify → extract → install → health check,不得依赖 UI 状态迁移表提供安全保证。 +3. 长期可把“安装事实”(not_installed/installed/update_available)与“操作状态”(queued/downloading/verifying/...)拆为两个维度,避免一个枚举同时表达持久事实和瞬时操作。 + +在没有拆分基础状态前直接删除这些迁移,会破坏取消更新和失败恢复语义。 + +#### P4 · 改为运维确认项 + +仓库已经存在 `.gitea/workflows/phase0-build.yml`,并复用 `verify_phase0.sh`;不能表述为“没有 CI”。 + +本地仓库只能确认 CI 定义存在,无法证明远端 runner 已启用和实际跑绿。准确结论应是: + +- 本地完整闸门已成立。 +- 远端 push/PR 强制执行状态尚未验证。 +- 后续应在 Gitea 确认 Actions、runner、日志留存和失败阻断策略。 + +#### P5 · 杂项修订 + +- `soft_quay.code-workspace` 如果确认只是个人编辑器配置,加入 `.gitignore` 合理;如果团队需要共享多 workspace/gopls 配置,则应先审查内容再决定提交。 +- 当前机器已实际验证 `go1.25.0` 与 `go1.20.14` 可用;剩余问题是远端 runner 能否获取对应工具链,不是本地 pin 是否存在。 + +### 原审核遗漏的问题 + +#### 新 P1 · Runtime Close/Publish 语义没有达到“生产级并发”结论 + +`core/application.Runtime.Publish` 的两个 `select` 不能保证 `done` 优先。以下交错是合法的: + +1. Publish 第一个 `select` 观察到 Runtime 尚未关闭。 +2. 另一个 goroutine 执行 Close 并关闭 `done`。 +3. Publish 进入第二个 `select`;若事件通道同时可写,`done` 与发送分支都 ready。 +4. Go `select` 随机选择 ready 分支,Publish 可能发送成功并返回 nil。 + +现有测试只覆盖“Close 完成后再 Publish”,没有覆盖 Publish 与 Close 并发。因此需要先明确契约: + +- 若允许已开始的 Publish 与 Close 竞争成功,应在注释和测试中明确。 +- 若要求 Close 取得线性化点后绝不再接受事件,则需要调整同步设计并增加并发/重复稳定性测试。 + +在该契约未明确前,应把原文“生产级并发质量”改为“基础并发骨架可用,关闭竞态仍需定义”。 + +#### 新 P2 · core 边界脚本只检查直接 import + +`scripts/check_core_boundaries.py` 使用 `go list -json ./...`,然后检查 core 包的 `Imports/TestImports/XTestImports`。它能可靠阻止 core 源码直接 import Gio、Windows、SQLite 和 app 层,但没有遍历 `go list -deps` 的完整传递依赖图。 + +当前 core 没有第三方模块依赖,因此现实边界仍然安全;原文“解析真实 import 图”的描述略宽。未来 core 引入第三方依赖时,应决定是否把传递依赖也纳入检查。 + +#### 新 P3 · 技术栈写了 vendor,仓库现实没有 vendor + +`docs/03-tech-stack.md` 将“版本锁定 + vendor”标为已定,依赖纪律也要求 Gio 相关依赖 vendor,但仓库没有 vendor 目录。 + +这不属于 T-001～T-004 的验收失败,但属于文档与现实不一致。后续需要二选一: + +- 正式增加适配双 workspace 的 vendor 流程和 CI 验证。 +- 在未实施前把技术栈状态改为“版本锁定;vendor 待实现”,避免宣称已有离线依赖保障。 + +### 建议处理顺序 + +1. 修正根 `go.work` 的 Win7 版本污染,同步调整双 workspace sync 和文档。 +2. 明确并测试 Runtime 的 Close/Publish 并发契约。 +3. 注释状态机中的“恢复基础状态”迁移,不要直接收紧为单一路径。 +4. 在远端 Gitea 验证 Actions runner 与 push/PR 阻断。 +5. 处理 vendor 文档差异,再决定 workspace ignore 和 UI token 是否需要独立任务。 + +## 交叉复核裁定(定稿) + +> 复核视角:Claude 全栈开发工程师,对 Codex 二次复核逐条核验后裁定。 +> 核验方式:grep 生产调用者、检查工作流文件与 vendor 目录、对照 `runtime.go` / `status_resolver.go` 实际代码。 +> 日期:2026-07-16 + +### 已确认的事实(可作为后续任务前提) + +| 项 | 结论 | 证据 | +| --- | --- | --- | +| 根 `go.work` Win7 版本污染 | 属实,未修复 | `go.work` 仍 `use ./app-win7`;根解析 Gio v0.10.1,`app-win7/go.work` 解析 v0.6.0 | +| CI 工作流存在 | 属实(原审核 P4 表述不准) | `.gitea/workflows/phase0-build.yml` 触发 `push`/`pull_request`,跑 `verify_phase0.sh`;远端 runner 实跑状态未验证 | +| 状态迁移表非安全门禁 | 属实 | `CanTransition`/`ValidateTransition` 无生产调用者,仅自身与测试引用;真实推导在 `core/domain/status_resolver.go:ResolveAppStatus`(facts 驱动,已分离操作态与安装事实) | +| Runtime Close/Publish 竞态 | 属实 | `core/application/runtime.go` 第二个 `select`:`done` 已关闭且 buffer 有空位时,`<-done` 与 `events<-event` 同时 ready,select 随机选,Publish 可能在 Close 后仍成功返回 nil;测试未覆盖并发 | +| core 边界脚本只查直接 import | 属实,现实安全 | `check_core_boundaries.py` 检查各 core 包 `Imports/TestImports/XTestImports`,不遍历第三方传递依赖;core 目前无第三方依赖,故无实际泄漏 | +| vendor 文档与现实不符 | 属实 | 无 `vendor/` 目录;`03-tech-stack.md` 将“版本锁定 + vendor”标为已定 | + +### 对 Codex 二次复核的采纳与修正 + +- **采纳** P1(workspace 隔离)及其实施细则:移除 app-win7 后 `verify_phase0` 必须对**两个** workspace 分别 `go work sync`;须验证 gopls 对嵌套 win7 workspace 的识别,避免从“版本污染”变成“win7 不在 workspace”。 +- **采纳** 新 P1(Runtime 竞态)、新 P2(边界传递依赖)、新 P3(vendor 漂移)三项遗漏。 +- **修正** P2 论述:调色板是 `image/color.NRGBA`(标准库),共享它**不需要** import Gio,可放独立非 Gio 包;“不值得为少量 token 建共享模块”的结论保留,但约束不成立于“会污染 core”。 +- **补充**(双方均遗漏):`allowedTransitions` + `CanTransition`/`ValidateTransition` 现为**测试脚手架/死代码**,与 `ResolveAppStatus` 并行且可能漂移,构成第二个合法状态来源。须接进 resolver/安装编排做校验,或显式标注“仅规格测试”。 +- **补充** CI 工具链隐藏依赖:`phase0-build.yml` 只 `setup-go 1.25.0`,win7 构建靠 `GOTOOLCHAIN=go1.20.14` **自动联网下载**才能绿,与“无 vendor”是同一可复现性缺口。须显式预装 go1.20.14 或知情接受该联网依赖。 + +### 最终处理顺序(定稿) + +1. **[结构 · 最高优先]** 修根 `go.work` Win7 版本污染:根仅 `use ./app-modern` + `./core`;`verify_phase0` / `init` 对根与 `app-win7/go.work` 分别 `go work sync`;更新 `03-tech-stack.md` “三模块根 workspace” 描述;验证 gopls 嵌套 workspace 识别。 +2. **[低成本 · 一并做]** 状态模型收口:给“恢复基础状态”迁移补注释与测试;同时裁定 `allowedTransitions`/`CanTransition` 去留(接入 resolver 校验 或 标注仅测试);**不**在拆分基础状态维度前直接删迁移。 +3. **[可复现性 · 一组收口]** vendor 与 CI 工具链一起处理:确定 vendor 落地或把技术栈状态改为“vendor 待实现”;`phase0-build.yml` 显式预装 go1.20.14 或知情接受自动下载;在远端 Gitea 确认 Actions runner、push/PR 阻断与日志留存。 +4. **[契约 · 可延后至 Phase 2-3]** 定义并测试 Runtime 的 Close/Publish 并发契约(接上真实事件消费者与关停语义后再定,含并发/重复稳定性测试)。 +5. **[观察项 · 非重构]** UI 适配持续保持“共享 ViewModel + 隔离 Gio 控件”纪律;边界脚本待 core 引入第三方依赖时再评估是否纳入传递依赖检查;`soft_quay.code-workspace` 确认为个人配置后加入 `.gitignore`。 + +> 裁定:Codex 二次复核整体成立且部分优于原审核;上述顺序为双方交叉复核后的共识。P1 为唯一“结构级、应尽快处理”项,其余为低成本收口、可复现性收口与可延后契约。