已完成截至3.21日所有新功能对新架构的适配#21
Conversation
# Conflicts: # .idea/workspace.xml # cmd/tui/main.go # internal/server/domain/memory.go # internal/server/domain/role.go # internal/server/infra/provider/modelscope_provider.go # internal/server/infra/repository/memory_repository.go # internal/server/service/chat_service.go # internal/server/service/memory_service.go # internal/server/service/role_service.go
- 目前仅包括到记忆和角色功能,未更新到最新内容 Closes #123
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
| return &securityConfigRepositoryImpl{} | ||
| } | ||
|
|
||
| func (r *securityConfigRepositoryImpl) LoadAll(configDir string) (*domain.Config, *domain.Config, *domain.Config, error) { |
There was a problem hiding this comment.
LoadAll 接收了 configDir,但后面完全没有用它;同时这里硬编码的还是 security/*.yaml,而这个 PR 真正提交的是 configs/security/*.yaml。结果就是安全模块默认读不到任何规则文件,只会悄悄回落到空配置。
| paramsMap = make(map[string]interface{}) | ||
| } | ||
|
|
||
| // 执行工具 |
There was a problem hiding this comment.
这里直接实例化并执行了 read/write/edit/bash/... 工具,但整个调用链里没有任何 SecurityService.Check(...)。也就是说这个 PR 虽然引入了安全模块和规则文件,实际执行时还是完全绕过了它,LLM 产出的工具调用会直接落地执行。
There was a problem hiding this comment.
收到,安全模块目前还没有投入使用
| func ruleMatches(rule domain.Rule, toolType string, target string) bool { | ||
| var pattern string | ||
|
|
||
| switch toolType { |
There was a problem hiding this comment.
这里按 Read / Write / Bash / WebFetch 只挑了目标字段做匹配,但没有检查规则里对应的 r / w / x / n 权限位。这样像 target: "src/**/*.go", r: allow 这种规则,在当前实现下同样会放行写操作,和 YAML / 文档里承诺的按权限区分完全不一致。
minorcell
left a comment
There was a problem hiding this comment.
这次 review 主要看了构建可用性、工程规范和安全边界。当前我建议先不要合并,主要原因有两类:
- 基础可用性还没过:我本地跑了
go test ./...和go vet ./...,目前都会失败,直接原因是多个占位info.go把目录打成了多包混用。 - 安全模块还没有真正落地:一方面规则文件路径读不到,另一方面工具执行链路也没有接入
SecurityService.Check(...),所以现在的实现和文档/PR 描述之间存在明显落差。
另外从工程规范看,这个 PR 仍然偏大(62 个文件、7k+ 行),commit 历史里也还有 大体结构、<feactor>、<fix> 这类不稳定或拼写错误的提交信息;而且 PR body 已经明确写了“占位文件下一次再修”,这类已知清理项通常不建议带着一起进主分支。详细问题我都写成 inline comments 了。
| { | ||
| "id": "1774011772051038000-failure_note", | ||
| "type": "fix_recipe", | ||
| "summary": "hello", |
There was a problem hiding this comment.
这个文件里已经不是配置模板,而是实际运行产生的记忆数据了,而且内容还是无关的对话/排序示例和乱码截断。data/ 作为本地运行产物本来就不应该进 PR;把这类测试/演示数据带进仓库,只会污染默认数据集并干扰后续调试。
There was a problem hiding this comment.
收到,提交的时候把之前的文件也一起推上去了
|
|
||
| import "encoding/json" | ||
|
|
||
| type Tool interface { |
There was a problem hiding this comment.
这个接口把执行语义和展示元数据绑在一起了,导致每个工具都要重复实现大量几乎恒定的 Name() / Description() 样板代码,后面还在返回结果时反复回调 Name()。当前调用方真正需要的核心能力其实只有执行;名称/描述更适合作为注册元数据而不是每个工具实例的方法。
|
补充两个没法挂到具体代码行上的工程问题:
|
| Target string `yaml:"target,omitempty"` | ||
| Command string `yaml:"command,omitempty"` | ||
| Domain string `yaml:"domain,omitempty"` | ||
| R string `yaml:"r,omitempty"` |
There was a problem hiding this comment.
这里用单个字母 R/W/X/N 作为 YAML 对外协议字段,语义过于隐晦,必须依赖额外文档才能知道分别代表什么。安全规则本身就是高风险配置,建议直接用可读的字段名(例如 read / write / exec / network),否则很容易配错也很难 review。
| cfg.Memory.StoragePath = "./data/memory_rules.json" | ||
| cfg.Memory.PersistTypes = []string{"user_preference", "project_rule", "code_fact", "fix_recipe"} | ||
| cfg.History.ShortTermTurns = 6 | ||
| cfg.Persona.FilePath = "./persona.txt" |
There was a problem hiding this comment.
默认配置这里指向 ./persona.txt,但这次 PR 实际提交的是 configs/persona.txt / configs/persona.txt.example。首次启动自动生成 config.yaml 后,这个默认路径会直接指向一个不存在的文件,导致人设注入功能静默失效。
There was a problem hiding this comment.
这里常见的行业做法是,把所有配置文件:模型配置、权限配置等都收敛到跟目录:
比如 claude code的配置就是这么做的:
cd ~
cd .claude/当然我不知道你们有没有理解这个说法?
此外,code review 之后如果有不懂的地方,我们是可以在这个 pr 里面继续交流的。 @phantom5099
| } | ||
|
|
||
| // Description 返回工具描述。 | ||
| func (b *BashTool) Description() string { |
There was a problem hiding this comment.
这里的描述是“持久的 shell 会话”,但下面实际实现是每次都单独起一个 bash -c 进程。这样前一次命令里的 cd、export 等状态根本不会保留,调用方如果按文案理解成 stateful shell,很容易写出错误的执行链。
| roleSvc domain.RoleService | ||
| //provider domain.ChatProvider | ||
| } | ||
|
|
There was a problem hiding this comment.
NewChatService 这里仍然接收 provider domain.ChatProvider,但结构体字段已经删掉了,下面 Send 里又重新 provider.NewChatProvider(req.Model)。这说明重构没有收尾干净:调用方现在还在白做一遍 provider 初始化,接口签名也会误导后续维护者。
merge: resolve conflicts for PR #315
refactor(memo): simplify memo tool argument/scope helpers
…lution fix(tui): resolve startup view compile break and restore tick flow
改动范围
未完成项