Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
openviking/session/compressor.py
Outdated
| best_skill_name = part_skill | ||
| best_status = part.tool_status or "completed" | ||
|
|
||
| if best_ratio >= 0.9 and best_skill_name: |
There was a problem hiding this comment.
[Bug] Skill 校准阈值不一致:这里使用 0.9 作为 skill 模糊匹配阈值,但 tool_skill_utils.calibrate_skill_name 使用的是 0.8。
memory_extractor.py 的 extract() 方法调用 calibrate_skill_name(阈值 0.8),而 compressor.py 的 _get_tool_skill_info 使用 0.9。两条代码路径对同一个 skill name 会产生不同的校准结果——相似度 0.85 的名称在 extract() 能匹配,在这里会失败。
建议统一为相同的阈值,或直接复用 calibrate_skill_name。
openviking/session/compressor.py
Outdated
| return (part_tool, "", part.tool_status or "completed") | ||
|
|
||
| ratio = SequenceMatcher(None, candidate_norm, part_norm).ratio() | ||
| if ratio > best_ratio or (ratio == best_ratio and ratio >= 0): |
There was a problem hiding this comment.
[Suggestion] 平局处理条件 ratio == best_ratio and ratio >= 0 中,ratio >= 0 对 SequenceMatcher 恒为 true(返回值范围 [0, 1]),实际效果是「同分时后出现的 part 总是覆盖前面的」。
从测试用例 test_best_match_tie_picks_most_recent_tool_part 来看这是有意为之,建议加注释说明这个设计意图(如 # tie-break: prefer the last occurrence),避免后续维护者误解。tool_skill_utils.py:64 存在同样的写法。
openviking/session/compressor.py
Outdated
| candidate_skill = candidate.skill_name | ||
| from difflib import SequenceMatcher | ||
|
|
||
| def _normalize(name: str) -> str: |
There was a problem hiding this comment.
[Suggestion] _get_tool_skill_info 重复实现了 tool_skill_utils.py 中的模糊匹配逻辑,包括这个局部 _normalize 函数(与 normalize_name 功能完全相同)以及 SequenceMatcher 匹配流程。
这种重复正是上面 skill 阈值不一致 bug 的根本原因。建议直接调用 calibrate_tool_name / calibrate_skill_name,避免逻辑分叉。
Description
Related Issue
Type of Change
Changes Made
Testing
Checklist
Screenshots (if applicable)
Additional Notes