Skip to content

Conversation

@ERIC961
Copy link
Contributor

@ERIC961 ERIC961 commented Nov 7, 2025

📝 Description | 描述

编辑交易员配置后,系统提示词模板(system_prompt_template)不生效


🎯 Type of Change | 变更类型

  • 🐛 Bug fix | 修复 Bug
  • ✨ New feature | 新功能
  • 💥 Breaking change | 破坏性变更
  • 📝 Documentation update | 文档更新
  • 🎨 Code style update | 代码样式更新
  • ♻️ Refactoring | 重构
  • ⚡ Performance improvement | 性能优化
  • ✅ Test update | 测试更新
  • 🔧 Build/config change | 构建/配置变更
  • 🔒 Security fix | 安全修复

📋 Changes Made | 具体变更

后端使用请求中的 SystemPromptTemplate 值
移除跳过已存在交易员的逻辑,确保配置同步
前端在更新请求中包含 system_prompt_template 字段
API 响应中包含 system_prompt_template,便于前端显示


🧪 Testing | 测试

  • Tested locally | 本地测试通过
  • Tests pass | 测试通过
  • Verified no existing functionality broke | 确认没有破坏现有功能

✅ Checklist | 检查清单

Code Quality | 代码质量

  • Code follows project style | 代码遵循项目风格
  • Self-review completed | 已完成代码自查
  • Comments added for complex logic | 已添加必要注释

Documentation | 文档

  • Updated relevant documentation | 已更新相关文档

Git

  • Commits follow conventional format | 提交遵循 Conventional Commits 格式
  • Rebased on latest dev branch | 已 rebase 到最新 dev 分支
  • No merge conflicts | 无合并冲突

📚 Additional Notes | 补充说明

English:中文:


By submitting this PR, I confirm | 提交此 PR,我确认:

  • I have read the Contributing Guidelines | 已阅读贡献指南
  • I agree to the Code of Conduct | 同意行为准则
  • My contribution is licensed under AGPL-3.0 | 贡献遵循 AGPL-3.0 许可证

🌟 Thank you for your contribution! | 感谢你的贡献!

@github-actions
Copy link

github-actions bot commented Nov 7, 2025

🤖 Advisory Check Results

These are advisory checks to help improve code quality. They won't block your PR from being merged.

📋 PR Information

Title Format: ✅ Good - Follows Conventional Commits
PR Size: 🟢 Small (15 lines: +9 -6)

🔧 Backend Checks

Go Formatting: ⚠️ Needs formatting

Files needing formatting
api/server.go

Go Vet: ✅ Good
Tests: ✅ Passed

Fix locally:

go fmt ./...      # Format code
go vet ./...      # Check for issues
go test ./...     # Run tests

⚛️ Frontend Checks

Build & Type Check: ✅ Success

Fix locally:

cd web
npm run build  # Test build (includes type checking)

📖 Resources

Questions? Feel free to ask in the comments! 🙏


These checks are advisory and won't block your PR from being merged. This comment is automatically generated from pr-checks-run.yml.

Copy link
Collaborator

@SkywalkerJi SkywalkerJi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

跳过交易员的逻辑是 @icyouo @Icyoung 看看会不会影响编辑回写,需不需要修改。

@xqliu
Copy link
Contributor

xqliu commented Nov 8, 2025

代码审查报告 - PR #708

审查结果:⚠️ 需要修复

业务层面审查

✅ 需求验证

  • 需求明确:编辑交易员后,系统提示词模板应该生效
  • 需求合理:用户修改配置后应立即生效

❌ 功能完整性问题

PR 包含3个修改,但存在逻辑问题:

修改1:API 接受 system_prompt_template 参数 ✅

// api/server.go
SystemPromptTemplate: req.SystemPromptTemplate, // 从请求中读取

修改2:前端发送 system_prompt_template ✅

// web/src/components/AITradersPage.tsx
system_prompt_template: data.system_prompt_template,

修改3:注释掉"已加载跳过"逻辑 ❌ 危险

// manager/trader_manager.go
// if _, exists := tm.traders[traderCfg.ID]; exists {
//     log.Printf("⚠️ 交易员 %s 已经加载,跳过", traderCfg.Name)
//     continue
// }

🔴 BLOCKING - 严重问题

问题1:注释掉的逻辑会导致重复加载

位置manager/trader_manager.go:781-785

问题描述

  • 注释掉"跳过已加载交易员"的逻辑后,LoadUserTraders 会重复加载同一个 trader
  • 可能导致:
    • 同一个 trader 在 tm.traders map 中被覆盖
    • 多个 goroutine 同时运行同一个 trader(如果之前的 trader 正在运行)
    • 资源泄漏(旧的 trader 实例没有被停止)

错误的修复方式

// ❌ 错误:直接注释掉跳过逻辑
// if _, exists := tm.traders[traderCfg.ID]; exists {
//     log.Printf("⚠️ 交易员 %s 已经加载,跳过", traderCfg.Name)
//     continue
// }

正确的修复方式

// ✅ 正确:先停止旧实例,再加载新配置
if oldTrader, exists := tm.traders[traderCfg.ID]; exists {
    log.Printf("⚠️ 交易员 %s 已存在,停止旧实例并重新加载", traderCfg.Name)
    
    // 停止旧的 trader
    if oldTrader.IsRunning() {
        oldTrader.Stop()
    }
    
    // 从 map 中删除旧实例
    delete(tm.traders, traderCfg.ID)
}

// 继续加载新配置...

问题2:LoadUserTraders 的调用时机不明确

疑问

  • LoadUserTraders 在什么时候被调用?
  • 是否在用户点击"保存"后立即调用?
  • 还是只在系统启动时调用?

需要验证

  • 如果只在启动时调用,那么注释掉跳过逻辑也不会生效(因为重启才会加载)
  • 如果在"保存"后调用,那么需要确保旧实例被正确停止

问题3:配置更新的正确流程

当前可能的流程

  1. 用户在前端修改配置
  2. 前端调用 handleUpdateTrader API
  3. 后端更新数据库
  4. ❓ 如何让内存中的 trader 实例使用新配置?

可能的解决方案

方案A:重启 trader(推荐)

// api/server.go handleUpdateTrader
func (s *Server) handleUpdateTrader(c *gin.Context) {
    // ... 更新数据库 ...
    
    // 如果 trader 正在运行,重启它以应用新配置
    if existingTrader, err := s.traderManager.GetTrader(traderID); err == nil {
        if existingTrader.IsRunning() {
            log.Printf("⚠️ 交易员 %s 配置已更新,重启以应用新配置", traderID)
            existingTrader.Stop()
            
            // 重新加载配置
            s.traderManager.RemoveTrader(traderID)
            newTrader, err := s.traderManager.LoadTrader(updatedConfig)
            if err == nil && req.Start {
                newTrader.Start()
            }
        }
    }
    
    c.JSON(http.StatusOK, gin.H{"message": "更新成功"})
}

方案B:热更新配置(复杂)

// trader/auto_trader.go
func (at *AutoTrader) UpdateConfig(newConfig AutoTraderConfig) error {
    at.mu.Lock()
    defer at.mu.Unlock()
    
    at.config = newConfig
    at.customPrompt = newConfig.CustomPrompt
    at.systemPromptTemplate = newConfig.SystemPromptTemplate
    // ...
    
    log.Printf("✓ 配置已更新(无需重启)")
    return nil
}

技术层面审查

✅ API 契约一致性

  • 前端发送 system_prompt_template
  • 后端接收 system_prompt_template
  • 后端返回 system_prompt_template(在 GET 接口中)

❌ 内存同步问题

  • 数据库更新后,内存中的 trader 实例未更新
  • 注释掉的逻辑可能导致重复加载

E2E 验证报告

❌ 测试场景1:编辑运行中的 trader

步骤1:用户修改 trader 的 system_prompt_template
步骤2:点击"保存"
步骤3:后端更新数据库
预期:trader 实例使用新的 system_prompt_template
实际:❌ 可能不生效(内存中的实例未更新)

⚠️ 测试场景2:编辑后重启 trader

步骤1:用户修改配置
步骤2:停止 trader
步骤3:重新启动 trader
预期:使用新配置
实际:⚠️ 需要验证 LoadUserTraders 是否在启动时调用

❌ 测试场景3:重复加载

场景:LoadUserTraders 被调用两次
第一次:加载 trader A(ID=123)
第二次:再次加载 trader A(ID=123)
预期:第二次跳过或替换第一次
实际:❌ 注释掉跳过逻辑后,可能导致重复加载

最小修复建议

方案1:在 handleUpdateTrader 中重启 trader(推荐)

// api/server.go
func (s *Server) handleUpdateTrader(c *gin.Context) {
    // ... 更新数据库 ...
    
+   // 如果 trader 正在运行,重启以应用新配置
+   existingTrader, err := s.traderManager.GetTrader(traderID)
+   wasRunning := err == nil && existingTrader != nil && existingTrader.IsRunning()
+   
+   if wasRunning {
+       log.Printf("⚠️ 交易员 %s 配置已更新,重启以应用新配置", traderID)
+       existingTrader.Stop()
+       s.traderManager.RemoveTrader(traderID)
+   }
+   
+   // 重新加载配置
+   newTrader, err := s.traderManager.LoadSingleTrader(traderID)
+   if err != nil {
+       c.JSON(http.StatusInternalServerError, gin.H{"error": "重新加载失败"})
+       return
+   }
+   
+   if wasRunning {
+       newTrader.Start()
+   }
    
    c.JSON(http.StatusOK, gin.H{"message": "更新成功"})
}

方案2:恢复跳过逻辑,但添加"强制重新加载"参数

// manager/trader_manager.go
-// if _, exists := tm.traders[traderCfg.ID]; exists {
-//     log.Printf("⚠️ 交易员 %s 已经加载,跳过", traderCfg.Name)
-//     continue
-// }
+if oldTrader, exists := tm.traders[traderCfg.ID]; exists {
+    log.Printf("⚠️ 交易员 %s 已存在,停止旧实例", traderCfg.Name)
+    if oldTrader.IsRunning() {
+        oldTrader.Stop()
+    }
+    delete(tm.traders, traderCfg.ID)
+}

总结

这个 PR 的出发点是正确的(让 system_prompt_template 可编辑),但实现有问题

优点

  1. ✅ API 契约完整(前后端都传递 system_prompt_template)
  2. ✅ 数据库会正确保存新值

严重问题

  1. ❌ 注释掉跳过逻辑可能导致重复加载
  2. ❌ 内存中的 trader 实例不会使用新配置
  3. ❌ 没有处理正在运行的 trader

建议

  • 不要合并当前 PR
  • 采用方案1(在 handleUpdateTrader 中重启 trader)
  • 或者实现热更新配置功能
  • 添加集成测试验证配置更新流程

审查结论:❌ 不通过,需要重新设计配置更新流程

@hzb1115
Copy link
Member

hzb1115 commented Nov 9, 2025

Pls fix this according to the comment otherwise this will be closed. Thanks!

@hzb1115
Copy link
Member

hzb1115 commented Nov 11, 2025

Pls recreate PR, closed

@hzb1115 hzb1115 closed this Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants