fix(core): surface provider persistence errors in management API

[hi-neason]

Summary

The project provider management HTTP handlers in core/management.go (activate, add, remove) called the configured persistence callbacks (SaveActiveProvider, AddProviderToConfig, RemoveProviderFromConfig, wired in cmd/cc-connect/main.go:913-931) with _ = and then returned HTTP 200 even when the on-disk write failed. The in-memory state changed but the TOML config was not updated, so the next restart silently reverted the user's add/remove/switch while the API had told them it succeeded.

The same inconsistency existed in core/engine.go's /provider chat command (_ = e.providerSaveFunc(provName)), even though its sibling paths — clear, switch, add, remove at engine.go:10474/10563/10604/10645/10727 — all log the save error properly.

Change

• In the management handlers, persist first and only mutate in-memory state if the save succeeds. On failure, log and return HTTP 500 so a non-2xx response means "nothing changed".
• For activate, look up existence before the save; return 404 for unknown providers.
• For /provider in the engine, replace _ = e.providerSaveFunc(...) with the same error-logging pattern used by its siblings.
• No behavior change for successful writes.

Type of change

• Bug fix (non-breaking change that fixes an issue)

Testing

Automated tests added in this PR

• core/management_provider_persist_test.go
  • TestMgmt_ActivateProvider_PersistFailure — expects 500 and that the active provider is unchanged.
  • TestMgmt_DeleteProvider_PersistFailure — expects 500 and that the in-memory provider list is unchanged.
  • TestMgmt_AddProvider_PersistFailure — expects 500 and that the new provider is not added to memory.
  • TestMgmt_ProviderLifecycle_HappyPath — sanity-checks 200 + correct mutations when persistence succeeds.

For bug fixes only — regression test

• Regression tests: the three ...PersistFailure tests above.
• Manual verification this test catches the regression:
  • Reverted the production fix locally; all three failed with status = 200, want 500 on persist failure.

Critical User Journeys (CUJ) impact

• F — config switching (/lang /provider /model reload): directly covers /provider and the equivalent management API.

• go test ./core/ -run TestCUJ passes locally.

• The /provider chat-command behavior on the success path is unchanged (still calls SetActiveProvider, resets sessions, saves).

Manual / user-visible behavior change

If persisting a provider change to config.toml fails (disk full, permission denied, read-only config), the management API now returns 500 with the underlying error instead of 200, and the runtime state is left untouched so the user can retry without a phantom divergence.

Checklist (reviewer will verify)

• go build ./... passes
• go test ./... passes (touched-package tests run with -tags no_web matching the Makefile default; -race not required — no new concurrency)
• AGENTS.md Pre-Commit Checklist items are satisfied
• No new hardcoded platform/agent names in core/
• i18n strings have all-language translations (no new user-facing strings — error messages are surfaced in the management API JSON response only)
• No secrets / credentials in source

Related

• Prior pattern at core/engine.go:10474, 10563, 10604, 10645, 10727 already handled these save errors correctly; this PR closes the four remaining call sites that did not.

[chenhg5]

结论: Approve

总体判断: 一个 critical correctness fix——provider 持久化错误之前被 _ = 吞掉，导致 HTTP 200 + 内存 state 已改 + TOML 没写，下一次重启 silent revert。这是「user-visible divergence」的严重 bug。修复后 memory mutation 仅在 persist 成功后才发生，且失败时返回 500 + 不修改 state。建议合入。

Review 范围:

• 看了 core/management.go 中 activate / add / remove handler 的改动（persist-first-then-mutate 模式 + 404 for unknown）。
• 看了 core/engine.go 中 /provider chat command 的 _ = e.providerSaveFunc(...) 改为 proper error logging（与兄弟路径对齐）。
• 看了新增 4 个测试：3 个 PersistFailure + 1 个 HappyPath。
• CI: run 31354787223 全绿。

✅ 做得好的地方:

• 修复 critical data consistency bug：原行为是「HTTP 200 → 内存 state 已改 → TOML 没改 → 下次重启 silent revert」。这是用户体验上最糟糕的 bug——API 说成功，实际失败，但用户看到的成功信号无法撤销。这是必须修的。
• persist-first-then-mutate 模式：这是 transactional pattern 的简化版——「先准备好 persistent state，再 commit in-memory state」。失败时两边都未变，回滚成本 0。
• 与既有 sibling 路径对齐：作者明确指出 engine.go:10474, 10563, 10604, 10645, 10727 已正确处理 save errors，只是 _ = e.providerSaveFunc(...) 一处漏了。这是「find the one missing link」型的 fix，比「重构整个 handler」风险小得多。
• 404 for unknown provider：activate 路径加 existence check，避免「添加不存在 provider → silent error」的 UX 问题。
• 测试覆盖三种失败路径 + happy path：ActivateProvider_PersistFailure / DeleteProvider_PersistFailure / AddProvider_PersistFailure + ProviderLifecycle_HappyPath。每个 PersistFailure 测试断言「HTTP 500 + memory unchanged」——这是修复的核心契约。
• PR body 描述清晰：明确指出「-race not required — no new concurrency」（避免 reviewer 多跑 race 检测浪费时间）、「error messages are surfaced in the management API JSON response only」（说明 i18n 不需要更新）。

🟠 建议改进（不阻塞）:

• activate 的 404 检查：404 vs 400 区分：当前对「unknown provider」返回 404 是合理的，但 client side 可能用 400（user error）vs 404（resource not found）区分。建议 PR body 或代码注释明确选择 404 的理由（resource = provider, semantically not found）。
• Failure error 信息是否暴露 internal details：return HTTP 500 with the underlying error——如果 underlying error 包含 path / 文件描述符等 internal 信息，可能 leak。建议确认 response body 只含 high-level 错误描述，把 full error 留到 server log。
• persist-first 失败但 caller 想知道为什么：目前是 log error + return 500。建议 management API 在 response body 里包含 {"error": "persist_failed", "detail": "<safe_summary>"}，让 web UI 能给用户具体提示。
• add 路径的 success path 没显式测成功添加后的 list 行为：happy path 测试 sanity-check，但没断言「添加成功 → list 返回新 provider」。低优先。

🔵 可选优化:

• 无。

Testing / Risk:

• 已看到的验证: 4 个新测试覆盖 3 失败路径 + 1 happy path；fix revert 后 3 个失败测试都红（作者 self-verify）；CI 全绿。
• 未覆盖风险: 真生产 disk full / read-only 场景的 integration test 需要 filesystem mock——目前用 fake SaveFunc func() error 模拟已经足够 unit-test。
• Blast radius: 仅 core/management.go + core/engine.go，对 client / protocol 无 breaking change。

Next step:

• 建议 owner 直接 merge。这是一个 critical correctness fix，scope 小、行为变化只影响「之前 silent failure 现在 loud failure」，不会破坏现有 client。可以现在合。