Conversation
dosubot
bot
added
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes persona marker handling by introducing a persona_utils module and implements persona inheritance for new conversations in the ConversationManager. When a new conversation is initiated without a specific persona ID, the system now attempts to inherit and normalize the persona from the current active conversation. A review comment identifies a potential performance improvement to eliminate redundant database queries that occur when both the caller and the new_conversation method attempt to resolve the persona ID independently.
| if persona_id is None: | ||
| curr_cid = await self.get_curr_conversation_id(unified_msg_origin) | ||
| if curr_cid: | ||
| curr_conv = await self.db.get_conversation_by_id(cid=curr_cid) | ||
| if curr_conv: | ||
| persona_id = normalize_persona_id(curr_conv.persona_id) |
There was a problem hiding this comment.
这段代码正确地实现了在创建新对话时继承当前人格的逻辑。
然而，我注意到这可能会导致一些调用路径（例如 conversation.py 中的 new_conv）出现效率问题。new_conv 会先调用 _get_current_persona_id 来解析当前的人格，然后将结果传给 new_conversation。如果当前人格是 [%None]，_get_current_persona_id 会返回 None，然后 new_conversation 会因为 persona_id is None 而再次执行几乎相同的逻辑来解析当前人格，这导致了重复的数据库查询。
虽然这不影响功能的正确性，但为了提高性能和代码的简洁性，建议在未来的重构中，让调用方（如 new_conv）直接调用 new_conversation() 而不传递 persona_id，完全依赖此处的继承逻辑。
由于相关调用方的代码不在此次变更范围内，这可以作为一个后续的改进点。
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since you introduced
is_persona_none_markerandnormalize_persona_id, consider using them consistently everywhere you compare againstPERSONA_NONE_MARKER(e.g., inpersona_mgr.resolve_selected_persona) to avoid duplicating marker-specific logic. is_persona_none_markeris currently not used anywhere in the diff; either replace directPERSONA_NONE_MARKERequality checks with this helper or drop the function to keep the helper module minimal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since you introduced `is_persona_none_marker` and `normalize_persona_id`, consider using them consistently everywhere you compare against `PERSONA_NONE_MARKER` (e.g., in `persona_mgr.resolve_selected_persona`) to avoid duplicating marker-specific logic.
- `is_persona_none_marker` is currently not used anywhere in the diff; either replace direct `PERSONA_NONE_MARKER` equality checks with this helper or drop the function to keep the helper module minimal.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
idiotsj
force-pushed
the
fix/persona-inheritance-minimal
branch
from
d69380a to
a0186b5
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
ConversationManager.new_conversation,normalize_persona_idis used but there’s no corresponding import shown in the diff; double-check thatastrbot.core.persona_utils.normalize_persona_idis imported inconversation_mgr.pyto avoid runtime errors. - The new behavior in
new_conversationtreatspersona_id=Noneas “inherit from current conversation,” which changes semantics for callers that previously passedNoneto explicitly mean no persona; consider either normalizing an explicitPERSONA_NONE_MARKERtoNonebefore persisting, or adding an explicit flag/constant to request a non-inherited, persona-less conversation to keep the API behavior unambiguous.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ConversationManager.new_conversation`, `normalize_persona_id` is used but there’s no corresponding import shown in the diff; double-check that `astrbot.core.persona_utils.normalize_persona_id` is imported in `conversation_mgr.py` to avoid runtime errors.
- The new behavior in `new_conversation` treats `persona_id=None` as “inherit from current conversation,” which changes semantics for callers that previously passed `None` to explicitly mean no persona; consider either normalizing an explicit `PERSONA_NONE_MARKER` to `None` before persisting, or adding an explicit flag/constant to request a non-inherited, persona-less conversation to keep the API behavior unambiguous.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
修完了，不再扩展了……如果要改逻辑另算吧 |
1 participant
Fixes #7045
Modifications / 改动点
修复
ConversationManager.new_conversation()在未显式传入persona_id时不会继承当前对话人格的问题将显式“无人格”标记
[%None]统一归一化为None，避免该标记被继续继承到新对话新增
astrbot/core/persona_utils.py，统一管理PERSONA_NONE_MARKER和 persona 归一化逻辑替换 persona 相关命令和管理器中的硬编码 marker 判断，避免后续值漂移
新增聚焦单测，覆盖以下场景：
[%None]不会被继承persona_id优先级高于自动继承This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
验证步骤：
/persona psychologistpersona_id的新建对话路径psychologist/persona unsetNone，而不是继续保留[%None]已执行的聚焦回归测试：
uv run pytest tests/unit/test_conversation_mgr.py tests/unit/test_astr_main_agent.py::TestGetSessionConv tests/unit/test_astr_main_agent.py::TestEnsurePersonaAndSkills -q ............. 13 passed, 3 warnings in 1.58sChecklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码.
Summary by Sourcery
Fix conversation persona handling so new conversations correctly inherit or clear persona settings and centralize the explicit no-persona marker logic.
Bug Fixes:
Enhancements:
Tests: