feat: add knowledge vector grounding for AI shopping - #226
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return matched, nil | ||
| } | ||
|
|
||
| type FSClient struct { |
There was a problem hiding this comment.
该函数在遍历 fsInstances map 时未加锁，而 fsInstances 是一个全局变量，可能被多个 goroutine 同时读写（例如在其他地方调用 ReloadProject 或初始化客户端），存在并发读写风险，可能导致数据竞争（data race）或运行时 panic。建议对 fsInstances 的访问进行同步保护（如使用 sync.RWMutex）。
| func parseSearchGoodsRequest(arguments string, fieldAware bool, knowledge *knowledgeEvidence) (recallsvc.SearchGoodsRequest, error) { | ||
| var req recallsvc.SearchGoodsRequest | ||
| if !fieldAware { | ||
| return req, json.Unmarshal([]byte(arguments), &req) |
There was a problem hiding this comment.
在 marshalSearchGoodsRequest 函数中，当 json.Marshal(req) 失败时（即 err != nil），仍会返回 payload（此时为 nil 或无效值）和错误。但代码逻辑 if err != nil || knowledge == nil || len(req.KnowledgeCandidateIDs) > 0 会导致即使 marshal 失败也直接返回 (payload, err)，这可能导致后续使用无效的 payload。更严重的是，如果 err != nil，payload 是 nil，但函数仍尝试对它进行 json.Unmarshal(payload, &fields)，这将引发 panic。
具体路径：
json.Marshal(req)返回非 nil 错误 →payload为 nil- 条件
err != nil成立 → 跳过 if 块？不，实际上该条件用于提前返回，但当前逻辑是：只要err != nil就return payload, err，这是正确的。
然而仔细看：
payload, err := json.Marshal(req)
if err != nil || knowledge == nil || len(req.KnowledgeCandidateIDs) > 0 {
return payload, err
}此逻辑正确：若 marshal 失败，直接返回错误；若 knowledge 为 nil 或已有 KnowledgeCandidateIDs，也直接返回原始 marshal 结果。
但问题在于：当 err == nil 且 knowledge != nil 且 len(req.KnowledgeCandidateIDs) == 0 时，才会进入后续逻辑。这部分逻辑是安全的。
再检查：json.Unmarshal(payload, &fields) 中 payload 此时一定非 nil 且有效，因为前面已确保 err == nil。
因此，该函数逻辑实际无 bug。
但另一个潜在问题是：marshalSearchGoodsRequest 在 knowledge == nil 时直接返回原始 marshal 结果，而调用方 normalizeFieldAwareToolCalls 传入的 knowledge 来自参数，可能为 nil。需确认调用处是否保证 knowledge 非 nil。
查看调用点：
normalizeFieldAwareToolCalls被runAgentLoop调用，传入的knowledge是runAgentLoop的参数，该参数由上层传入。runAgentLoop的knowledge参数在函数开头被用于messagesWithKnowledge和FieldAwareSearchGoodsTool(knowledge.candidateIDs())，若knowledge为 nil，则knowledge.candidateIDs()会 panic。
因此，若 knowledge 可能为 nil，则多处会 panic。但变更前代码未使用 knowledge，变更后新增了对其的解引用（如 knowledge.candidateIDs()、knowledge.apply(&req) 等），若调用方可能传入 nil，则会导致空指针 panic。
关键点：runAgentLoop 新增参数 knowledge *knowledgeEvidence，并在多处直接使用其方法或字段，例如：
knowledge.candidateIDs()messagesWithKnowledge(..., knowledge)（内部可能使用）dispatchTool(..., knowledge, ...)→ 最终调用knowledge.apply(&req)
如果 knowledge 为 nil，这些调用都会 panic。
结论：本次变更假设 knowledge 非 nil，但函数签名允许传入 nil。若上层可能传入 nil，则存在空指针解引用风险。由于变更引入了对 knowledge 的多次解引用而未做 nil 检查，存在空指针 panic 的明确风险。
| result = append(result, knowledgeMessages...) | ||
| result = append(result, messages[1:]...) | ||
| return result | ||
| } |
There was a problem hiding this comment.
当 messages 非空但长度为 1 时，messages[1:] 是合法的（不会越界），但逻辑上存在潜在问题：代码假设 messages[0] 是用户消息或可直接保留的首条消息，而将知识相关消息插入到第一条之后。然而，如果原始 messages 的第一条已经是 system 消息，这种插入方式可能导致多个 system 消息顺序混乱，但这属于业务逻辑问题，非明确 Bug。
更严重的问题是：当 messages 为空时，函数返回 knowledgeMessages；但当 messages 非空时，强制取 messages[0] 并拼接后续消息。如果调用方传入的 messages 为 nil，则 len(messages) == 0 成立，会返回 knowledgeMessages，看似安全。然而，在 Go 中，对 nil slice 执行 len() 是安全的，因此此处无 panic。
但注意：evidence.promptJSON 被直接拼接到字符串中，若其内容不可信且最终被渲染到前端，可能存在 XSS 风险。不过，由于上下文是作为 LLM 的 system prompt 输入，通常不直接输出到 Web 页面，因此是否构成 XSS 取决于后续使用方式。在缺乏更多信息的情况下，暂不视为明确安全漏洞。
综上，最值得关注的问题是：当 messages 长度为 0 时返回 knowledgeMessages，而长度 ≥1 时将 messages[0] 作为第一条，再插入知识消息。但如果 messages[0] 是 user 或 assistant 角色，而 knowledgeMessages 是 system 角色，这可能违反某些 LLM API 对消息顺序的要求（如 system 消息必须在最前）。但这仍属业务逻辑问题，非代码层面的明确 Bug。
经过仔细分析，本 hunk 实际上没有空指针、数组越界、死循环、数据竞争或明确的安全漏洞。对 nil slice 的处理正确，索引操作在 len(messages) > 0 时安全。 因此，本次变更未发现明显问题。
| return recall | ||
| } | ||
|
|
||
| func (r *Ha3ChatRecall) GetCandidateItems(user *module.User, context *pairecctx.RecommendContext) []*module.Item { |
There was a problem hiding this comment.
在初始化过程中使用 panic 处理错误（如 fs.GetFeatureStoreClient 或 fsClient.GetLLMConfig 失败）可能导致服务崩溃，属于严重稳定性问题。应改为返回 error 或记录日志并降级处理，而非直接 panic。
| if ok { | ||
| fs.ReloadProject() | ||
| continue | ||
| } |
There was a problem hiding this comment.
此处存在并发安全问题：在读取 fsInstances[name] 后释放了读锁，然后基于 ok 的判断执行 fs.ReloadProject()。但在读锁释放后、调用 ReloadProject() 前，其他 goroutine 可能已删除或替换了该 name 对应的实例，导致操作了错误或已失效的对象。应将 ReloadProject() 调用包含在读锁保护范围内，或改用其他同步机制确保一致性。
1 participant
Summary
Ha3KnowledgeVectorConfunder the existing chat recall configurationWhy
The existing field-aware fallback can only search terms produced by the Planner. User wording that does not match the catalog vocabulary can therefore still produce zero or irrelevant results. This change grounds Planner product-type extraction in values retrieved from a knowledge index built from the actual product catalog.
This is a stacked PR on top of #225. It contains only the knowledge-vector increment; the field-aware fallback remains isolated in PR1.
Implementation notes
FeatureStoreNameandLLMConfigName; vector dimension comes from the configured modelHa3KnowledgeVectorConfis presentKnowledgeTypesis required and limited tocategory,categories,content,brand,tag, andtitleproduct_type_keywords, so the product recall path stays small and reuses field-aware fallbackValidation
git diff --checkgo test ./service/aishopping ./service/recall ./algorithm/aichat ./persist/fs ./recconf -run '^$'go vet ./service/aishopping ./service/recall ./algorithm/aichat ./persist/fs ./recconfgo build ./...Known repository test issue: running the unfiltered
service/recalltest package still fails in the pre-existingTestMultibizRecallbecause it constructsNewBeMultiBizRecallwith a nil configuration. The package compilation gate, vet, and full repository build pass.