feat: install command + webchat cron fix + gateway TurnCount persist#555
Conversation
hotplex-ai
left a comment
There was a problem hiding this comment.
Code Review — hotplex-ai
Verdict: APPROVE | P0:0 P1:0 P2:0 P3:1
Summary
PR 包含三个独立功能:install 命令(~100 行重复代码提取)、webchat cron 类型修复、gateway TurnCount 持久化。代码质量高,安全加固到位(PowerShell 单引号转义、shell 参数转义、filepath.Clean 归一化)。
Findings
- [P3]
internal/updater/updater.go:311—findChecksum中filepath.Base(parts[1])检查放在parts[1] == filename之前,将更常见的精确匹配作为快速路径放前面更优:parts[1] == filename || filepath.Base(parts[1]) == filename。纯微优化,无功能影响。
Observations (non-blocking)
SQLiteStore缺少var _ TurnQuerier = (*SQLiteStore)(nil)编译时接口检查(pgStore已有)。预存问题,非此 PR 引入,建议后续补上。formatDuration在page.tsx和detail/page.tsx中重复定义(~20 行),可考虑提取为共享 util。非阻断。
Verified
- ✅
TurnQuerier接口已添加LatestTurnNum,SQLite/PG/mock 三实现均更新 - ✅ SQL 查询含
generation过滤,避免跨代污染 - ✅
AppliedResetGen字段已添加到sessionAccumulator,bridge.go + bridge_forward.go 幂等逻辑一致 - ✅ PowerShell
-notcontains+ 单引号转义防止通配符/注入 - ✅
shellEscape()+strings.NewReplacer处理 Unix shell 注入 - ✅
CronSchedule类型修复与后端 API 对齐 - ✅
findChecksum容忍路径前缀,向后兼容旧 checksums.txt - ✅ Release workflow checksum 合并为单步,
cd dist避免路径前缀
hotplex-ai
left a comment
There was a problem hiding this comment.
Code Review — hotplex-ai
Verdict: REQUEST_CHANGES | P0:1 P1:0 P2:1 P3:0
P0 — Must Fix
[P0] PowerShell AddToUserPath double-negative logic inversion — PATH never added (confidence: 92)
internal/cli/install/install.go AddToUserPath (Windows branch):
if(-not ($p -split ';' -notcontains 'DIR'))Trace:
$p -split ';'→ array of PATH entries$array -notcontains 'DIR'→$truewhen DIR absent,$falsewhen present-not ($true)→$false→ if-block skipped when DIR absent-not ($false)→$true→ if-block runs when DIR present
Result: PATH is modified (re-added) when dir is already present, and NOT modified when dir is absent. Completely inverted.
Fix: Remove outer -not:
`if($p -split ';' -notcontains '%s'){`P2 — Nit
[P2] formatDuration duplicated between cron page and detail page (confidence: 90)
Identical formatDuration(ms: number): string defined in both:
webchat/app/admin/cron/page.tsxwebchat/app/admin/cron/detail/page.tsx
Extract to shared utility (e.g., webchat/lib/utils/cron.ts).
Positive Notes
- Install command package is well-structured with proper atomic copy, SHA256 verification, and cross-platform support
- DRY improvement: wizard.go duplicate functions replaced by
install.*package - Webchat CronJob types now correctly match Go backend struct (CronSchedule object, payload nesting, epoch ms timestamps)
- TurnCount persistence with
LatestTurnNum+AppliedResetGenidempotent guard is solid updater.gofindChecksumnow tolerates path-prefixed filenames (aligns with release.yml consolidation)shellEscapefor Unix PATH manipulation is correct
hotplex-ai
left a comment
There was a problem hiding this comment.
Code Review — hotplex-ai
Verdict: APPROVE | P0:0 P1:0 P2:2 P3:0
4-agent parallel review completed (Bug/Security + Compliance + History/Compat + SOLID/Perf).
Findings
[P2] formatDuration duplicated in two cron admin pages — webchat/app/admin/cron/page.tsx and webchat/app/admin/cron/detail/page.tsx both define identical formatDuration(ms) helpers. Extract to a shared utility (e.g., webchat/lib/utils/format-duration.ts) to prevent divergence. [CONFIDENCE:85]
[P2] Silent error discard on new LatestTurnNum DB call (internal/gateway/bridge_forward.go) — tn, _ := b.turnsQuerier.LatestTurnNum(...) discards DB errors without logging. While consistent with the existing LatestGeneration pattern nearby, a b.log.Debug("turns: restore turn num", "error", err) would aid diagnostics for rare DB failures. [CONFIDENCE:78]
Summary
Clean PR with well-structured changes. The install command correctly extracts shared utilities from the onboard wizard with proper atomic binary copy and cross-platform PATH management. The cron admin type fixes properly resolve React error #31 with correct TypeScript interfaces matching the Go backend. TurnCount persistence uses a sound AppliedResetGen idempotency guard. Release workflow consolidation is correct. No security, concurrency, or data integrity issues found.
hotplex-ai
left a comment
There was a problem hiding this comment.
Code Review — hotplex-ai
Verdict: REQUEST_CHANGES | P0:0 P1:1 P2:6 P3:0
P1 — 必须修复
- [P1] cron detail 编辑保存时 schedule 序列化为 string 而非 CronSchedule object (
webchat/app/admin/cron/detail/page.tsx:116)
handleSave将schedulestate(已被formatScheduleStr转为"cron:0 9 * * *"字符串)直接作为updates.schedule发送到 API。但后端CronJob.schedule类型已变更为CronScheduleobject ({kind, expr, ...})。
影响: 用户通过 admin UI 编辑 cron job 的 schedule 后,API 会收到 string 而非 object,导致保存失败或数据损坏。
修复建议: state 中保持CronScheduleobject,仅在渲染层调用formatScheduleStr。
P2 — 建议修复
-
[P2] sessionAccumulator data race 扩展 (
internal/gateway/bridge_forward.go:141-146)
本 PR 将AppliedResetGen加入processForwardedEvent中 lock 外的 mutation(预存模式)。 返回裸指针后,字段修改不在accumMu保护内。非本 PR 引入根因,但扩大了竞态面。 -
[P2] LatestTurnNum 失败仅 Debug 日志 (
internal/gateway/bridge_forward.go:91-95)
DB 查询失败时acc.TurnCount保持 0,静默丢失 turn 计数。建议提升到 Warn 级别。 -
[P2] wizard.go error wrapping 丢弃 %w 链 (
internal/cli/onboard/wizard.go:1272,1299,1319)
重构后 wizard.go 用"update failed: " + err.Error()拼接错误,丢失了 install 包内部的fmt.Errorf("%w")错误链。应改为fmt.Errorf("update failed: %w", err)。 -
[P2] 测试中硬编码路径分隔符
/(internal/cli/install/install_test.go:2121-2122,2143,2163-2165,2200)
多处使用dir+"/dst"而非filepath.Join(dir, "dst"),Windows 测试会失败。 -
[P2] duplicate formatSchedule 函数 (
webchat/app/admin/cron/detail/page.tsx:36vswebchat/app/admin/cron/page.tsx:17)
两个近似的 schedule 格式化函数,应提取到共享 utility。 -
[P2] adminFetch 丢弃 202 响应 body (
webchat/lib/api/admin-client.ts:77)
HTTP 202 现在与 204 同等处理返回undefined。需确认无 admin endpoint 返回 202+body。
| schedule !== job.schedule || | ||
| message !== job.message || | ||
| schedule !== formatScheduleStr(job.schedule) || | ||
| message !== (job.payload?.message ?? '') || |
There was a problem hiding this comment.
[P1] schedule serialization bug: handleSave sends schedule as a formatted string (e.g. "cron:0 9 * * *") but the backend API expects a CronSchedule object ({kind: "cron", expr: "0 9 * * *"}).
Fix: Keep the original CronSchedule object in state, use formatScheduleStr only for display:
const [scheduleObj, setScheduleObj] = useState<CronSchedule>(...)
// render: {formatScheduleStr(scheduleObj)}
// save: updates.schedule = scheduleObj
hotplex-ai
left a comment
There was a problem hiding this comment.
Code Review — hotplex-ai
Verdict: APPROVE | P0:0 P1:0 P2:1 P3:0
P2
- [P2] 架构 HTML 引用已移除的 Codex CLI 适配器 (
docs/architecture/hotplex_architecture.html:1182-1184) — CLAUDE.md 明确标注 OpenCode CLI 已被 OCS 替代,但新增的架构交互图仍展示Codex CLI和worker/codexcli节点。建议移除或合并为 OCS 说明。
审查摘要
PR 包含三大功能块,整体质量良好:
-
hotplex install命令 — 从 wizard.go 提取共享 install 包,DRY 改善显著。原子文件复制(temp+rename)、SHA256 内容校验、跨平台 PATH 管理(zsh/bash/fish/PowerShell)实现稳健。10 个 table-driven 测试覆盖关键路径。 -
Webchat CronSchedule 修复 — CronJob 类型从扁平 string 字段正确迁移到嵌套结构(CronSchedule/CronPayload/CronJobState),修复 React #31 错误。
parseScheduleStr+formatScheduleStr双向转换正确处理了序列化问题。 -
TurnCount 持久化 —
LatestTurnNumSQL 查询正确使用COALESCE(MAX(), 0)避免sql.ErrNoRows。AppliedResetGen幂等保护确保 ResetSession 和 processForwardedEvent 两路径均安全。SQL placeholder 通过已有Rebind机制兼容 PG。
已验证的安全项
- PowerShell 单引号字符串阻止表达式注入 ✓
shellEscape正确处理单引号转义 ✓CopyBinarytemp 文件失败时正确清理 ✓findChecksumfilepath.Base兼容新旧 checksum 格式 ✓context.Background()与 forwardEvents 已有模式一致 ✓
Add top-level `hotplex install` command to install the binary to PATH. Extract shared binary installation utilities from onboard wizard into internal/cli/install package, eliminating ~100 lines of duplication. Safety fixes (from code review): - DefaultInstallDir returns error instead of empty string - PowerShell/shell command injection protection via proper escaping - IsInPATH uses filepath.Clean for trailing-slash normalization - SHELL unset produces clear error instead of confusing message - findChecksum tolerates path-prefixed filenames in checksums.txt - Release workflow checksums consolidated into single post-merge step Closes #553
- Check os.UserHomeDir() error in AddToUserPath (was silently discarded, causing RC file to be written in CWD when HOME is unset) - Replace PowerShell -notlike with -split/-notcontains to avoid treating [ and ] as wildcard characters in install paths
TurnCount was in-memory only, lost on gateway restart, causing duplicate turn_nums in the same generation. Fix by: - Loading latest turn_num from DB when TurnCount is 0 (post-restart) - Adding AppliedResetGen field to prevent duplicate generation increments on concurrent ResetSession calls - Adding LatestTurnNum query to TurnQuerier interface (SQLite + PG)
…as child
The schedule field was a CronSchedule object {kind, every_ms} but was
treated as string in admin cron pages, causing React to throw when
rendering it directly as a child element.
- Add CronPayload and CronJobState interfaces - Replace flat fields (message, runs_count, next_run_at, last_run_at) with nested payload/state objects matching the Go CronJob model - Update cron list and detail pages to use new type shape - Fix formatTime/formatDateTime to accept millisecond timestamps
- Fix handleSave to send payload object instead of flat message field - Handle 202 Accepted in adminFetch alongside 204 No Content
…creen Use minmax(0,1fr) instead of 1fr to prevent long prompt text from expanding the Name column beyond the container width, which pushed all other columns out of view.
- Extract formatDuration to shared util, deduplicate cron pages - Add debug log for LatestTurnNum DB error in bridge_forward - Swap findChecksum check order for fast path optimization
- P1: detail page schedule edit sends CronSchedule object instead of string - P1: add parseScheduleStr to convert edited text back to CronSchedule - P2: elevate LatestTurnNum error log from Debug to Warn
4878169 to
b7cc7a8
Compare
Summary
hotplex installcommand to install binary to PATH with security hardeningTurnCountacross gateway restartsChanges
Install Command (
#553)internal/cli/install/install.go— shared binary install utilities with security hardeninginternal/cli/install/install_test.go— 10 table-driven testscmd/hotplex/install.go—hotplex installcobra command (--path,--force)cmd/hotplex/main.go— registernewInstallCmd()internal/cli/onboard/wizard.go— use shared package, remove duplicate functionsinternal/updater/updater.go—findChecksumtolerates path-prefixed filenames.github/workflows/release.yml— consolidate checksum generationWebChat Cron Admin Fix
webchat/lib/types/admin.ts— addCronScheduleinterface, fix type fromstringto objectwebchat/app/admin/cron/page.tsx— addformatSchedule()/formatDuration()helperswebchat/app/admin/cron/detail/page.tsx— proper schedule object handling in edit/displayGateway Fix
TurnCountacross gateway restartsTest plan
make buildcompilesgo test ./internal/cli/... -race— all pass including 10 new testshotplex install— installs to~/.local/binhotplex install(again) — detects "already installed"hotplex install --force— forces reinstallnext build— TypeScript check passesCloses #553