visionA/local-tool/.autoflow/05-implementation/reviews/review-m8-10a-builtin-data-dir-fix.md

# Code Review 報告 — M8-10a P0 Latent Bug 修復（built-in data dir 解析）

## 審查摘要

- **審查對象**：`server/main.go`（新增 `resolveBuiltInDataDir` helper + `main()` 拆分 `builtInDataDir` / `dataDir` 兩個變數）
- **關聯檔**：`server/internal/flash/service.go`（接收參數由 `dataDir` → `builtInDataDir`，無 body 變動）
- **產出 Agent**：Backend
- **審查結果**：⚠️ **需修 1 個 Major（Linux AppImage 布局未覆蓋）後通過**
- **問題統計**：Critical: 0 / Major: 1 / Minor: 2 / Suggestion: 2

---

## 獨立驗證結果

| 驗證項 | 指令 | 結果 |
|-------|------|------|
| Build | `cd server && go build -o /tmp/rv-server .` | ✅ PASS |
| Vet | `cd server && go vet ./...` | ✅ PASS |
| Test | `cd server && go test -count=1 ./...` | ✅ 全綠（api / api/handlers / api/ws / device / model 5 個有測試的 package 全部通過） |
| Smoke (bundle) | 直接跑 `.../Contents/Resources/bin/visiona-local-server -port 3801 -data-dir $(mktemp -d)` | ✅ `Built-in data dir: .../Contents/Resources/data` → `Loaded 15 built-in models` → `GET /api/models` 回 15 個 model，`kl520-yolov5-detection` 的 `filePath` = `data/nef/kl520/kl520_20005_yolov5-noupsample_w640h640.nef`，bundle 實體 .nef 檔存在（7,506,224 bytes，與 catalog `modelSize: 7200000` 為人工估值合理吻合） |
| Smoke (dev mode) | 把 binary 放到 `server/` 下跑 `./rv-server-tmp -port 3802 -data-dir $(mktemp -d)` | ✅ `Built-in data dir: /Users/jimchen/visionA/local-tool/server/data` → `Loaded 15 built-in models` → `GET /api/models` 回 15 個 model |

**修復前的 bug 無法在修復後重現**：不管 Wails 傳什麼 `--data-dir`，`builtInDataDir` 都從 bundle 內解析，`/api/models` 保證回 > 0 個 model。

---

## 所有候選布局覆蓋狀態

| 布局 | binary 位置 | data 位置 | 命中候選 | 覆蓋狀態 |
|------|-----------|-----------|---------|---------|
| **macOS bundle** | `Contents/Resources/bin/visiona-local-server` | `Contents/Resources/data/` | `<base>/../data` | ✅ 已驗證 |
| **Dev mode（go run / 直接在 server/ 下）** | `server/visiona-local-server`（或 cwd） | `server/data/` | `<base>/data` | ✅ 已驗證 |
| **Windows installer**（Inno Setup，`installer/windows/visiona-local.iss` L72/L86）| `{app}\bin\visiona-local-server.exe` | `{app}\data\` | `<base>/../data` | ✅ 理論命中（未在 Windows 機器實跑，但 Inno 布局 = `base = {app}\bin`，`<base>/../data = {app}\data`，路徑對齊） |
| **Linux AppImage**（`installer/linux/build-appimage.sh` L29-33, L80）| `$APPDIR/usr/bin/visiona-local-server` | `$APPDIR/usr/lib/visiona-local/data/` | **無**（三個候選算出來分別是 `/usr/bin/data`、`/usr/data`、`/usr/Resources/data`，沒一個對） | ❌ **不覆蓋 — 見 Major-1** |

---

## 文件符合性檢查

這次是 P0 bug fix（既有行為修復），依 Autoflow S 級規則不需新增 PRD/TDD 條目。修復與現有 TDD §架構決策（bundle 內唯讀 vs user home 可寫分離）一致——事實上修復才讓程式碼行為與 TDD 的決策真正對齊（修復前是文件對但程式碼錯）。

---

## 問題清單

### Critical（必須修復，阻擋交付）

無。

### Major（必須修復，但不阻擋 macOS 初步交付）

| # | 檔案 | 行數 | 問題描述 | 建議修改方式 |
|---|------|------|----------|-------------|
| M-1 | `server/main.go` | 95-112 | `resolveBuiltInDataDir` 的三個候選沒有覆蓋 Linux AppImage 布局。AppImage 把 server binary 放 `usr/bin/`、data 放 `usr/lib/visiona-local/data/`（見 `installer/linux/build-appimage.sh` L29-33, L80），三個現有候選 `<base>/data`、`<base>/../data`、`<base>/../Resources/data` 都不會命中。結果：Linux 版 AppImage 啟動後 built-in catalog 依然載入 0 個 model，等於這個 P0 bug 在 Linux 上**沒被修掉**。另外 `AppRun` 已經 export `VISIONA_BUNDLE_LIB_DIR=${HERE}/usr/lib/visiona-local`（L134），server 目前完全沒讀這個 env var。**備註**：同樣的缺失其實也存在於先前就有的 `resolveBridgeScript`（L60-78），不是本次 PR 引入的 regression，但應同時修掉。 | 建議兩層保險同時做：①在 `resolveBuiltInDataDir` 開頭優先讀 `VISIONA_BUNDLE_LIB_DIR` env（若非空且 `<env>/data/models.json` 存在則直接回 `<env>/data`）；②新增第 4 個候選 `<base>/../lib/visiona-local/data`（對齊 FHS 布局，未來其他 Linux 打包方式也能用）。`resolveBridgeScript` 同步比照修掉，避免 Linux 版 Kneron bridge 也找不到。 |

### Minor（建議修復）

| # | 檔案 | 行數 | 問題描述 | 建議修改方式 |
|---|------|------|----------|-------------|
| m-1 | `server/main.go` | 110-111 | 三個候選全部沒命中時回的 fallback 是 `filepath.Join(base, "data")`，不是 `filepath.Abs(...)` 後的絕對路徑，與成功路徑回的是絕對路徑不一致（成功分支有 `filepath.Abs(c)`）。雖然 `model.NewRepository` 還是能用相對路徑讀檔，但 log 裡印出來的 `Built-in data dir: ...` 會是相對路徑，讓除錯變難。 | fallback 也過一次 `filepath.Abs`：`if abs, err := filepath.Abs(filepath.Join(base, "data")); err == nil { return abs }; return filepath.Join(base, "data")`。 |
| m-2 | `server/main.go` | 95-112 | 函式沒 log 自己試了哪幾條路徑。落到 fallback 時 `NewRepository` 只會印 `Warning: could not load models from <path>`，使用者看不到「我還試了另外兩個位置都沒中」。bug reporting 體驗不佳。 | 在 fallback 分支 `return` 前用 `log.Printf`（或注入 logger）印出嘗試過的三個候選路徑，或在函式簽名改成接收 logger。 |

### Suggestion（非必要，改善建議）

| # | 檔案 | 行數 | 建議內容 |
|---|------|------|----------|
| s-1 | `server/main.go` | 95-112 | `resolveBuiltInDataDir` 和 `resolveBridgeScript`（L60-78）兩個函式結構幾乎一模一樣（都是「一組候選 → 找存在的 → fallback 回第一個」）。可以抽一個通用 `findFirstExisting(candidates []string, sentinel string) (string, bool)`，讓兩者各自只定義候選清單和 sentinel 檔案（`models.json` vs `kneron_bridge.py`），減少未來「改了一個忘了改另一個」的風險。 |
| s-2 | `server/main.go` | 146-149 | `dataDir` 在 `cfg.DataDir == ""` 時 fallback 到 `builtInDataDir`（= bundle 內 read-only 目錄），但這只服務 dev mode。實際執行時 sentinel (`.first-ws-connected`)、custom-models、logs 都可能想寫進去，而 bundle 內目錄在 prod 是 read-only。雖然 dev mode 下通常有寫權限，但把「writable user dir fallback 到 read-only bundle dir」這件事放進註解說清楚比較好，避免未來有人以為這是正常的 prod 行為。 |

---

## 邊界情況檢查

| 情境 | 行為 | 評估 |
|------|------|------|
| `os.Executable()` 失敗 | `baseDir` 回 `"."`，`resolveBuiltInDataDir` 試 `./data`、`../data`、`../Resources/data`，其中任一有 `models.json` 就命中 | ✅ 合理，接力自然 |
| 三個候選都沒命中 | 回 `<base>/data`，`model.NewRepository` 印 warning，`r.models = []`，server 繼續跑 | ✅ 容錯策略合理（見 Minor m-2：若能多印「我試過的路徑」更好） |
| 目錄對但 `models.json` 損毀（JSON 解析錯）| `os.Stat` 判定命中 → `NewRepository` `json.Unmarshal` 印 warning → 0 個 model | ✅ 合理，不會誤判成「找別的目錄」 |
| 目錄對但 `models.json` 是 0 bytes | `os.Stat` 判定命中（non-dir）→ `NewRepository` `json.Unmarshal` 失敗印 warning → 0 個 model | ✅ 同上 |
| `models.json` 是個目錄（奇怪但可能發生） | `info.IsDir()` 為 true，跳過該候選繼續試下一個 | ✅ 已處理 |
| `cfg.DataDir` 指定為不存在路徑 | **只影響 writable 部分**：custom-models 讀取會 warning 但 empty、sentinel 寫入失敗但 WS Hub 會跳過、logs 可能寫不出。built-in catalog 不受影響（依然從 `builtInDataDir` 讀） | ✅ 修復後行為比修復前好 —— 修復前一個無效 `--data-dir` 會同時把 built-in catalog 弄掛 |
| `cfg.DataDir` 指定為路徑但 parent 不存在 | 同上 | ✅ |

---

## 其他 dataDir 誤用檢查（Grep server/ 全專案）

我 grep 了 `dataDir` / `DataDir` / `models.json` / `nef/kl` / `data/nef` 的所有出現位置，確認：

- `server/internal/api/ws/hub.go`：`sentinelDataDir`（用於 `.first-ws-connected` sentinel）→ ✅ 語意正確，寫入 writable user dataDir
- `server/internal/config/config.go`：`DataDir` CLI flag → ✅ 沒改
- `server/internal/flash/service.go`：`s.dataDir` 解析 `data/nef/...` 相對路徑 → ✅ 本次修復已把 `flash.NewService` 改吃 `builtInDataDir`，語意對齊
- `main.go` 其他位置（sentinel / custom-models / WS Hub）→ ✅ 都正確用 writable `dataDir`，沒有誤用

**沒找到其他地方把「應屬 bundle 內 read-only 的檔案」錯用 writable `dataDir` 解析。** 本次修復清單完備。

---

## 優點

1. **做法對齊既有範式**：`resolveBuiltInDataDir` 的候選路徑邏輯（1: dev flat / 2: installer / 3: macOS bundle）和既有 `resolveBridgeScript` 一致，未來維護者不需學新模式，讀一個就懂兩個。
2. **語意清晰拆分**：`builtInDataDir`（read-only, bundle 內）和 `dataDir`（writable, user home）兩個變數名 + 兩段 comment block 把意圖講得很清楚，修復前後讀 code 一看就懂「為什麼要兩個」。`model.NewRepository` 和 `flash.NewService` 傳的是 `builtInDataDir`、`custom-models` / `WS sentinel` 傳的是 `dataDir`，分工直觀。
3. **Dev mode 不 regression**：`cfg.DataDir == ""` 時 fallback 回 `builtInDataDir`，讓 `go run ./server` 繼續可跑 —— 重要，因為它同時讓 reviewer 這次能在 `server/` 目錄下獨立驗證 dev mode。
4. **命中條件嚴格**：用「`models.json` 存在且非目錄」當 sentinel（而不是 `data/` 目錄存在就行），避免選到 wails build artifact 留下的空 `data/` 目錄（這正是修復前誤入的那個坑）。
5. **容錯好**：三候選都沒命中時 fallback 而非 panic，`NewRepository` 也只印 warning 不 crash，符合「不 regression、壞的優雅」。
6. **增量最小**：唯一改動檔就是 `server/main.go`（+40 −6），沒動 `flash/service.go` 的 body，風險面小。
7. **Build/vet/test 全綠，獨立 smoke（bundle + dev mode 兩種路徑）也全綠**。

---

## 總結意見（≤400 字）

M8-10a 這個 P0 latent bug 的修復方向正確、做法乾淨，唯一改動檔 `server/main.go` 把 `builtInDataDir`（read-only bundle 內）與 `dataDir`（writable user home）拆成兩個語意明確的變數，並新增 `resolveBuiltInDataDir` helper 以 `models.json` 存在為命中條件自動跨 dev / installer / macOS bundle 三種布局。Flash service 同步改吃 `builtInDataDir`，避免相對路徑被誤解到 user home。Build / vet / test / smoke（bundle + dev mode）四項獨立驗證全綠，`/api/models` 穩定回 15 個 model，實體 .nef 檔路徑可對到 bundle 內。

**但 Linux AppImage 布局（`usr/bin/visiona-local-server` + `usr/lib/visiona-local/data/`）三個候選都不命中（Major-1）**，等於這個 P0 bug 在 Linux 上繼續潛藏——雖然同樣的缺失 `resolveBridgeScript` 先前就存在、並非本次 PR 引入。修復建議：①讀 `VISIONA_BUNDLE_LIB_DIR` env（`AppRun` 已 export）②加第 4 候選 `<base>/../lib/visiona-local/data`。兩個 Minor（fallback 沒 abs 化、沒 log 試過的路徑）和兩個 Suggestion（抽公用 helper、dev mode writable fallback 註解）屬改善性質，不阻擋交付。

**結論：⚠️ 需修 1 個 Major（Linux AppImage 覆蓋）後可交付；如果本次交付明確只限 macOS + Windows 兩平台，Major-1 可降級為「Linux 平台列為 known issue 追蹤到 M9」並直接放行。建議優先採前者，連 `resolveBridgeScript` 一起修掉，避免兩個函式分開欠債。**

---

## 第二輪 Review（2026-04-15）

### 審查對象

第一輪所有 Major / Minor / Suggestion 的修復 —— 唯一改動檔 `server/main.go`（+119 / −24 行）。

### 第一輪問題處理狀態

| # | 問題 | 狀態 | 驗證 |
|---|------|------|------|
| **Major-1** | Linux AppImage 布局未覆蓋（三候選全不命中） | ✅ **解決** | 候選清單補兩層：①env `VISIONA_BUNDLE_LIB_DIR/data`（AppRun 已 export）② FHS `<base>/../lib/visiona-local/data`。情境 2（模擬 AppImage 布局 + env var）→ log `Built-in data dir: .../usr/lib/visiona-local/data`、`Loaded 1`、`GET /api/models` total=1 ✅；情境 3（同布局但不設 env，靠 FHS 候選 5 命中）→ 同樣成功 ✅。`resolveBridgeScript` 同步修掉，連帶把第一輪備註的技術債清掉。 |
| **Minor-1** | fallback 沒 `filepath.Abs` 化 | ✅ **解決** | L145-149、L103-107：兩個函式 fallback 都過 `filepath.Abs`，絕對路徑能 err 時才退成 Join 的相對路徑（絕不 crash）。全不命中情境 log 印出 `Built-in data dir: /var/.../data`（絕對路徑）✅。 |
| **Minor-2** | 沒 log 試過的候選路徑 | ✅ **解決** | L142、L100：`log.Printf("warn: ... not found. Tried: %v", tried)`；`findFirstExisting` 每個候選都先 `filepath.Abs` 再丟進 `tried`，log 出來是絕對路徑清單，debug 訊息清楚。全不命中情境實測 log 輸出完整 4/5 個候選絕對路徑 ✅。 |
| **Suggestion s-1** | 抽 `findFirstExisting` helper | ✅ **解決** | L53-73 實作 `findFirstExisting(candidates, sentinel) (string, []string)`，`resolveBuiltInDataDir`（L139）和 `resolveBridgeScript`（L97）都改用。未來改動兩個函式的共通邏輯只需改一處。 |
| **Suggestion s-2** | dev mode writable fallback 加註解 | ✅ **解決** | L180-190：5 行 comment block 明確寫出「dev mode-only fallback、production 下 Wails 永遠會傳 `--data-dir`、若 packaged 模式沒傳則 writable 操作 log warning 不 crash」。意圖表達清楚。 |

### 獨立複驗結果

| 驗證項 | 指令 | 結果 |
|-------|------|------|
| Build | `cd server && go build -o /tmp/rv2-server .` | ✅ PASS |
| Vet | `cd server && go vet ./...` | ✅ PASS |
| Test | `cd server && go test -count=1 ./...` | ✅ 全綠（api / handlers / ws / device / model 5 個 test package 全通過）|
| 情境 2：AppImage + env var | 模擬 `usr/bin/<server>` + `usr/lib/visiona-local/data/models.json`，`VISIONA_BUNDLE_LIB_DIR=<appdir>/usr/lib/visiona-local` | ✅ `Built-in data dir: .../usr/lib/visiona-local/data` → `Loaded 1` → `GET /api/models` total=1 |
| 情境 3：AppImage FHS fallback | 同上布局但**不**設 env var | ✅ 候選 5 `<base>/../lib/visiona-local/data` 命中，同樣 `Loaded 1` → total=1 |
| 情境：全不命中 | 把 server binary 放到 `/tmp/xxx/` 獨立目錄，無 env | ✅ 兩個函式 log 出完整 `Tried:` 清單（都是絕對路徑），fallback 回絕對路徑，server 繼續跑 0 models（不 crash） |
| AppImage 對齊檢查 | 讀 `installer/linux/build-appimage.sh` L29-33 / L124-138 | ✅ `APPDIR/usr/bin/visiona-local-server` + `APPDIR/usr/lib/visiona-local/{data,scripts}` + AppRun export `VISIONA_BUNDLE_LIB_DIR=${HERE}/usr/lib/visiona-local` —— 與 server 候選 ①（env 路徑）和 ⑤（FHS 路徑）雙雙對齊 |
| cwd 變動風險檢查 | grep 專案找 `os.Chdir` / `Chdir(` | ✅ 零匹配 —— `./scripts` 相對候選在執行期不會因 cwd 漂移而解析錯 |

### 候選優先順序誤命中檢查

逐平台驗證新候選 `<base>/../lib/visiona-local/data` 不會在非 Linux 場景誤命中：

| 平台 | `<base>` | 候選 5 解析 | 該路徑實際是否存在 | 風險 |
|------|---------|-----------|----|------|
| macOS bundle | `Contents/Resources/bin` | `Contents/Resources/lib/visiona-local/data` | ❌（macOS bundle 不用 FHS） | 無 |
| Windows installer | `{app}\bin` | `{app}\lib\visiona-local\data` | ❌（Inno Setup 放 `{app}\data`） | 無 |
| Dev mode | `server/` 或 `.` | `../lib/visiona-local/data` | ❌ | 無 |
| Linux AppImage | `usr/bin` | `usr/lib/visiona-local/data` | ✅ | 正是目標 |

候選順序把 `<base>/data`、`<base>/../data`、`<base>/../Resources/data` 排在 FHS 之前，所有既有布局都會在進 FHS 前命中。**沒有誤命中風險。**

### 新觀察的邊界情況

| # | 觀察 | 評估 | 建議 |
|---|------|------|------|
| E-1 | `findFirstExisting` 簽名 `(string, []string)`，呼叫端用 `dir != ""` 判斷 OK | Go 慣例通常是 `(value, bool)` 或 `(value, error)`；現行簽名把「hit/miss」和「嘗試清單」合併一個回傳值，讀起來略非 idiomatic，但因為 `tried` 在 success path 沒用但無害、miss path 才拿去 log，合併簽名避免多一個回傳值，權衡合理 | 不阻擋通過。若要更 idiomatic 可改 `(dir string, tried []string, ok bool)`，但目前 call site 寫法（`if dir, tried := ...; dir != ""`）已經夠清楚 |
| E-2 | `resolveBridgeScript` / `resolveBuiltInDataDir` 的 warn 走 std `log.Printf`，其他資訊走 `pkglogger.logger.Info` —— log 格式不一致 | resolve 函式在 `main.go` 被呼叫的時序上 logger 已存在（L155 `pkglogger.New()`；resolve 在 L177、L236），**其實可以注入 logger**；但本次修改保留 std log 有個好處：若將來把 resolve 函式移到更早（在 logger 初始化前），不用再改簽名。**不影響正確性。** | 不阻擋通過。建議追蹤到下次重構 logger 時一起統一（加 Minor 備忘） |
| E-3 | `VISIONA_BUNDLE_LIB_DIR` 只在 Linux AppImage AppRun 設定 —— 其他平台若使用者手動設錯，是否會誤命中？ | `findFirstExisting` 對每個候選做 `os.Stat(.../sentinel)`，env 指到不存在的路徑會 Stat 失敗自然跳下一個候選；env 指到存在且含正確 sentinel 的路徑，代表使用者有意識地 override，行為合理 | 無風險。Resolve 函式的 `os.Stat` sentinel check 已是自然保險 |
| E-4 | `./scripts` 相對路徑候選（resolveBridgeScript 候選 6）在理論上會隨 cwd 漂移 | Grep 專案零匹配 `os.Chdir`，server 啟動流程不改 cwd，且相對候選放最後（env / base 相關的絕對候選都不命中才會走到），實務上只在純 dev mode cwd 對齊時觸發 | 無風險 |

### 新發現問題

**Critical：無。**

**Major：無。**

**Minor（非阻擋）：**

| # | 檔案 | 行 | 問題 | 建議 |
|---|------|----|------|------|
| m2-1 | `server/main.go` | 100, 142 | 兩個 resolve 函式的 warn log 用 std `log.Printf` 而非 `pkglogger.logger.Warn`，與後續 `logger.Info("Built-in data dir: ...")` 格式不一致 | 下次重構 logger 時把 logger（或至少 pkglogger）注入 resolve 函式，統一格式。不影響本輪通過。 |

**Suggestion：**

| # | 檔案 | 行 | 建議 |
|---|------|----|------|
| s2-1 | `server/main.go` | 59-73 | `findFirstExisting` 若改 `(dir string, tried []string, ok bool)` 可讓 call site 更 idiomatic（`if dir, tried, ok := ...; ok { ... } else { log; fallback }`）。非必須。 |

### 優點（第二輪新增）

1. **修復完整度超出第一輪要求**：第一輪只要求 Major-1（Linux 資料目錄覆蓋），Orchestrator 把所有 Minor + 兩個 Suggestion 一併做掉，且把第一輪**備註**的技術債 `resolveBridgeScript` 也同步修了，避免「兩個函式結構一樣但一修一未修」的分裂狀態。
2. **候選順序保守**：FHS 候選放在既有候選之後，零誤命中風險（見上表）；既有行為零 regression。
3. **`findFirstExisting` 抽象得當**：回傳 `tried` 清單給 log 用的設計雖然非完全 idiomatic，但對 debug 訊息清晰度貢獻很大（見全不命中情境 log 輸出）—— 這個權衡合理。
4. **註解品質高**：每個 resolve 函式的候選清單都有編號 + 情境說明；`dataDir` fallback 的註解把「dev-only / production Wails 永遠傳 / 壞了不 crash」三件事講清楚，未來維護者不會誤解。
5. **情境 2 / 3 雙路命中**：env var 和 FHS 兩層保險，即使未來 AppRun 的 env var 被移掉或名稱改了，FHS 還能兜底；反之亦然 —— 防禦性設計好。
6. **所有驗證全綠**：build / vet / test / 3 個布局實跑（bundle 第一輪已驗 / AppImage-env / AppImage-FHS / 全不命中 fallback）全通過。

### 總結意見（≤300 字）

Orchestrator 本輪把第一輪的 Major-1 + Minor-1/2 + Suggestion s-1/s-2 全數處理，且連帶修掉備註中 `resolveBridgeScript` 的同樣欠債，新增 `findFirstExisting` 共用 helper 讓兩個函式結構一致。候選清單補 env var（`VISIONA_BUNDLE_LIB_DIR`，AppRun 已 export）+ FHS 雙層保險，對 macOS bundle / Windows installer / Dev mode 零誤命中風險。三個布局實跑（AppImage + env var / AppImage FHS fallback / 全不命中 fallback）全綠，log 明確列出嘗試過的絕對路徑，fallback 也已 `filepath.Abs` 化。Build / vet / test 全綠。新觀察到的 2 個 Minor / Suggestion（std log vs pkglogger 格式一致性、helper 簽名 idiomatic 度）屬改善性，不阻擋交付。修復完整度超出第一輪最低要求，技術債清理徹底。

**結論：✅ 通過（可交付三平台：macOS bundle / Windows installer / Linux AppImage）。** 追蹤到下次 logger 重構時統一 std log 與 pkglogger 的格式（Minor m2-1）。