Conversation
chore(config): update llm.toml.example MCP server entries - Add allowed_tools restriction for GitHub MCP server - Mark arxiv server disabled by default, update image name - Add fetch and openweather MCP server examples - Update prts_wiki to public ghcr.io image with data volume mount
- DailyMessageCollector: per-group JSONL accumulation, deleted after generation - DailySummaryStore: standalone SQLite (data/daily_summaries.db) - DailySummaryEnabledGroups: opt-in JSON set, default off - LLM model cascade with @default sentinel and fallback chain - Scheduled generate at 06:00, publish at 12:00 via APScheduler - /summary on|off|status|now command (admin-only; now RPM=1) - DailySummaryConfig dataclass parsed from [daily_summary] in llm.toml - daily_summary added to SWITCHABLE_RULES
3aKHP
left a comment
There was a problem hiding this comment.
Code Review 综合评语
总体而言,PR 的功能设计清晰,模型级联策略和 opt-in 群开关的思路都很合理,JSONL + SQLite 的存储分层也是恰当的选择。但在正确性、架构合规和安全性上存在若干需要修复的问题,建议在合并前至少解决所有"高"和"中"级别的问题。
必须修复(高严重度)
1. 架构违规:适配层被业务层/平行适配层反向依赖(2处)
group_messages.py直接 importdaily_summary_plugin.record_group_message,后者在模块顶层触发 SQLite 初始化和文件 I/O。这破坏了三层分离,使得任何 importgroup_messages的场景(包括测试)都会执行副作用。record_group_message应下沉到quickquip/chat/daily_summary.py,singleton 实例化移到quickquip/app/的管线组装阶段。
2. 并发竞态:/summary now 冷却检查非原子
_on_cooldown+_mark_triggered之间存在 TOCTOU 窗口,两条同时到达的命令可能双双通过检查,各自触发一次 LLM 调用。需要用asyncio.Lock保护原子的"检查-标记"操作。
3. 串行 job 阻塞事件循环
_job_generate_summaries对所有群串行await,多群时会长时间占用 asyncio 事件循环,影响 NoneBot2 消息处理响应性。应改用asyncio.gather并发执行。
建议修复(中严重度)
4. 路径穿越风险
group_id未经格式校验即用于拼接文件路径,存在路径穿越风险。应校验为纯数字字符串。
5. Prompt Injection
- 昵称对照表(
name_table)和消息正文(text)未经任何清洗直接注入 LLM prompt。恶意昵称或消息可构造 prompt injection 攻击。至少需要过滤换行符和截断长度。
6. SQLite 连接未显式关闭
_connect()创建的连接在with conn:退出后未关闭(sqlite3的上下文管理器只管事务,不关闭连接)。高并发下会积累未释放的文件描述符。
7. JSONL 文件非原子写入风险
DailySummaryEnabledGroups.save()直接覆盖写入,进程崩溃时可能产生损坏文件,导致静默重置群开关集合。应使用临时文件 +os.replace()原子写入。
8. 原始消息文件删除时机过早
- 生成成功后立即删除 JSONL 文件,但若发布失败后无法重新生成。应将删除操作推迟到发布成功之后。
9. _job_publish_summaries 的日期计算在 missed-job 补跑时行为错误
- 依赖当前时间推算目标 summary_date,在非预期时间触发时会发布错误日期的摘要。建议在 store 中增加
published状态字段,发布时查询未发布记录而非用时间推算。
10. cron 时间描述硬编码 :00
/summary on的确认回复中用.split()[1] + ":00"显示时间,当 cron 分钟字段非 0 时会显示错误时间。
11. 消息长度截断缺失
- 大量消息被无截断地拼入 user message,可能超出模型 context window,导致整条 cascade 全部失败。
可选改进(低严重度)
12. /summary now 与定时 job 的窗口范围不一致(有意为之则需在文档中说明)
13. /summary now 与 _generate_and_store 约 25 行重复逻辑,建议提取公共函数消除重复。
14. 命令处理器使用连续 if 而非 if/elif,存在未来删去 finish() 后 fallthrough 的隐患。
15. min_messages / summary_length_hint 缺少下界校验(例如 min_messages=0 会对空群触发 LLM 调用)。
16. config/llm.toml.example 中的 model 名称为虚构值,应加注释说明是占位符;cron 时区(硬编码北京时间)应在注释中明确。
17. plugins/daily_summary.py shim 暴露了适配层 singleton,应只 re-export 业务层类。
18. quickquip/chat/daily_summary.py 三个职责合一,可考虑后续拆分为独立文件(非阻塞)。
| with path.open("a", encoding="utf-8") as f: | ||
| f.write(line + "\n") | ||
| except OSError: | ||
| logger.warning("daily_summary: failed to write message for group %s", group_id) |
There was a problem hiding this comment.
[并发/正确性 · 高] record() 直接以追加模式 open 文件写入,没有任何文件锁。NoneBot2 的事件处理器在同一个 asyncio 事件循环中以协程方式并发运行,虽然 f.write(line + "\n") 本身是同步的,但多个协程同时执行到这里会交给线程池(asyncio.to_thread / loop.run_in_executor)——如果以后改成异步写入,或者两条消息同时到达同一群,最终写入的行可能发生交错,导致 JSONL 损坏。
建议:使用 asyncio.Lock(每 group_id 一把),或者改用 aiofiles + 异步锁;最简单的临时方案是在模块级维护一个 threading.Lock 字典(每个 group_id 一把),在 with lock: 块内完成 open/write。
| path = self._file_path(group_id, local_date) | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| line = json.dumps({"sender": sender_name, "text": text, "ts": ts_val}, ensure_ascii=False) | ||
| try: |
There was a problem hiding this comment.
[安全性 · 中] group_id 直接被用于拼接文件路径 self.base_dir / str(group_id) / ...,没有做任何格式校验。如果 group_id 来自用户侧(例如转发或伪造事件),包含 ../ 等路径穿越字符时,会在 base_dir 之外创建/读取文件。
建议:在写入/读取前验证 group_id 是一个纯数字字符串:
gid = str(group_id)
if not gid.isdigit():
raise ValueError(f"invalid group_id: {gid!r}")或者至少用 Path(self.base_dir / gid).resolve() 与 self.base_dir.resolve() 做前缀检查(chroot 校验)。
|
|
||
|
|
||
| class DailySummaryStore: | ||
| """SQLite store for persisting generated daily summaries.""" |
There was a problem hiding this comment.
[资源管理 · 中] _connect() 每次调用都创建一个新的 sqlite3.Connection,而且没有 check_same_thread=False。SQLite 默认以 check_same_thread=True 打开,在多线程/多协程环境下从不同线程调用同一个连接对象会抛出异常。这里虽然每次都新建连接,但 _init_db 中 with self._connect() as conn: 结束时连接不会被显式关闭——sqlite3.Connection 的上下文管理器只管理事务(commit/rollback),并不关闭连接。
建议:
- 在
with块退出后显式调用conn.close(),或改写为:
conn = self._connect()
try:
conn.execute(...)
conn.commit()
finally:
conn.close()- 或使用连接池(
contextlib.closing包裹,或第三方aiosqlite)。
| """) | ||
| conn.commit() | ||
|
|
||
| def upsert( |
There was a problem hiding this comment.
[资源管理 · 中] upsert() 和 get() 同样存在连接未显式关闭的问题(见上条评论)。特别是 get() 中的 with self._connect() as conn: 结束后连接句柄悬挂,在高并发时可能积累大量未关闭的文件描述符。
此外,upsert() 里的 conn.commit() 是多余的——with conn: 上下文退出时若无异常会自动 commit;显式调用 commit 无害,但容易误导读者以为这是必要的。建议统一选择一种风格并加注释说明。
| self._groups: set[str] = set() | ||
| self.load() | ||
|
|
||
| def load(self) -> None: |
There was a problem hiding this comment.
[正确性 · 中] DailySummaryEnabledGroups.save() 在 add() / remove() 中被同步调用,每次变更都会触发文件写入。在高频操作(如批量恢复)或文件系统异常时,如果写入中途进程崩溃,可能留下半写状态的 JSON 文件,导致下次 load() 解析失败并静默重置为空集合(except (OSError, json.JSONDecodeError): self._groups = set())。
建议:采用原子写入模式——先写临时文件,再 os.replace() 替换:
import os, tempfile
tmp = self.path.with_suffix(".tmp")
with tmp.open("w", encoding="utf-8") as f:
json.dump({"enabled": sorted(self._groups)}, f, ensure_ascii=False, indent=2)
os.replace(tmp, self.path)| image = "mcp/fetch" | ||
|
|
||
| [[mcp.servers]] | ||
| id = "openweather" |
There was a problem hiding this comment.
[配置模板 · 中] model_cascade 中的模型名称(gemini-3.1-pro-high、gpt-5.4)看起来是虚构的示例值,但没有任何注释说明这些是占位符。新用户可能直接使用这份配置然后遇到 "provider not found" 或 API 错误,而不知道需要修改。
建议:
- 将示例值改为明确的占位符格式,例如
"your-provider-id/your-model-name"; - 或在注释中注明 "以下为示例,请替换为实际配置的 provider id 和 model name";
"@default"的语义也应在注释中解释(当前注释只在下方 TOML 注释中有说明,但离model_cascade列表较近的地方没有)。
| [[mcp.servers]] | ||
| id = "arxiv" | ||
| transport = "docker" | ||
| enabled = false |
There was a problem hiding this comment.
[配置模板 · 低] [daily_summary] section 缺少对 generate_cron / publish_cron 时区的说明。代码中硬编码使用 BEIJING_TIMEZONE(Asia/Shanghai),但 TOML 文件里没有任何提示。如果部署在境外服务器、系统时区非 CST 的用户看到这份配置,无法判断 cron 表达式对应的是服务器本地时间还是北京时间。
建议:在注释中明确说明,例如:
# cron 时间均为北京时间(Asia/Shanghai),与服务器系统时区无关
generate_cron = "0 6 * * *"
plugins/daily_summary.py
Outdated
| @@ -0,0 +1,15 @@ | |||
| from quickquip.adapters.nonebot.daily_summary_plugin import ( | |||
There was a problem hiding this comment.
[架构合规 · 中] 此 shim 直接从 quickquip.adapters.nonebot.daily_summary_plugin 导出 collector、store、enabled_groups、record_group_message、setup——这些都是适配层的实现细节,不应该作为 plugin shim 的公开接口暴露出去。其他 shim(如 plugins/message_stats.py、plugins/repeat_detector.py)re-export 的是业务层(quickquip/chat/)的公开对象。
这里把适配层的 singleton 暴露给 plugin 层,模糊了层次边界:如果外部代码通过 from plugins.daily_summary import collector 直接使用收集器,绕过了 app/ 层的管线组装,未来重构时会引入隐式耦合。
建议:shim 仅 re-export 业务层对象(DailyMessageCollector、DailySummaryStore 等类来自 quickquip.chat.daily_summary),不暴露 setup 和适配层 singleton。
| daily_cfg.generate_cron, | ||
| daily_cfg.publish_cron, | ||
| ) | ||
|
|
There was a problem hiding this comment.
[正确性 · 低] /summary status 和未知子命令(fallthrough 到最后的用法提示)分支使用的是 if args in {"status", "状态", ""} 而非 elif。由于每个 if 块都以 await summary_cmd.finish(...) 结尾(finish 会抛出 FinishedException 终止处理器),逻辑上不会有 fallthrough 问题,但这种风格容易让读者误以为多个分支会连续执行。
建议:改用 if/elif/elif/elif/else 结构以明确互斥语义,提升可读性,并避免未来有人在某个分支末尾删去 finish() 时引入意外的 fallthrough bug。
| @@ -0,0 +1,180 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
[代码质量 · 低] quickquip/chat/daily_summary.py 同时承担了三种职责:JSONL 消息收集(DailyMessageCollector)、SQLite 摘要存储(DailySummaryStore)、群开关状态管理(DailySummaryEnabledGroups)。随着功能迭代,这个文件会持续增长。参照项目现有的模式(chat/ 层每个关注点单独成文件,如 repeat_detector.py、rule_switch.py),建议将这三个类拆分到独立文件:
quickquip/chat/daily_collector.py—DailyMessageCollectorquickquip/chat/daily_store.py—DailySummaryStorequickquip/chat/daily_enabled_groups.py或在现有rule_switch.py中扩展 —DailySummaryEnabledGroups
当然这可以作为后续重构,不一定需要在本 PR 完成。
H1: move daily_collector/store/enabled_groups singletons and
record_group_message() to app/message_pipeline (app layer),
eliminating cross-adapter dependency and SQLite init at adapter
import time.
H2: add comment confirming asyncio single-threaded atomicity for
check-then-mark cooldown in /summary now handler.
H3: use asyncio.gather() for concurrent multi-group generation and
publish jobs instead of sequential awaits.
M1: add _safe_group_id() path-traversal guard (digit-only check)
for JSONL file construction.
M2: wrap chat log in === 聊天记录开始/结束 === delimiters and add
prompt-injection warning to system prompt.
M3: replace "with conn:" (transaction-only) with explicit try/finally
conn.close() in all DailySummaryStore methods.
M4: use atomic tmp-file rename in DailySummaryEnabledGroups.save().
M5+M6: add published_at column with schema migration; publish job
uses get_unpublished() instead of date arithmetic; JSONL deletion
moved to _publish_one() after confirmed delivery.
M7: add _cron_to_hhmm() helper for human-readable HH:MM in /summary
on confirmation message.
M8: add _truncate_chat_log() (300k char cap, keep tail, trim to
newline boundary) with truncation note injected into prompt.
L2: extract _run_generation() shared helper used by both scheduled
job and /summary now, eliminating duplicate LLM call logic.
L3: replace chained if/if/if with if/elif/elif/elif/else in command
handler.
L4: apply max(1, ...) / max(100, ...) lower-bound guards on
min_messages and summary_length_hint during TOML parsing.
L5: add comments to llm.toml.example clarifying cron timezone and
that model names are deployment-specific examples.
L6: rewrite plugins/daily_summary.py shim to re-export singletons
from app.message_pipeline instead of the adapter layer.
…in test context nonebot_plugin_apscheduler raises ValueError (not ModuleNotFoundError) when NoneBot has not been initialized. This happens when test_tz.py imports tz_tracker_plugin, which now transitively imports daily_summary_plugin. Broaden the except clause to cover both cases.
功能概述
实现每日群聊总结功能:凌晨 06:00 自动收集前一日(06:00–06:00 窗口)群聊记录,调用 LLM 生成约 2000 字小作文风格总结,中午 12:00 定时发布到群内。
新增模块
quickquip/chat/daily_summary.pyDailyMessageCollector(JSONL 原始消息读写)、DailySummaryStore(SQLite 摘要持久化)、DailySummaryEnabledGroups(群级开关 JSON)quickquip/llm/summarize.pygenerate_daily_summary():模型级联调用 + prompt 构建quickquip/adapters/nonebot/daily_summary_plugin.py/summary命令处理plugins/daily_summary.py关键设计
group_messages.py消息管道末尾挂 hook,将含 AT 格式化的rendered_text追加写入data/daily_msgs/{group_id}/{YYYY-MM-DD}.jsonl;生成成功后删除原始文件model_cascade列表逐个尝试,失败降级到下一个;支持"@default"占位符指向当前群绑定模型data/daily_summary_groups.json维护 opt-in 群集合(与rule_switch默认开启语义不同);/summary on/off同步更新两者/summary now(管理员,RPM=1)生成前一天 06:00 至当前时刻的总结,仅回复不入库data/daily_summaries.db,一张summaries表,UNIQUE(group_id, summary_date)配置示例(
llm.toml)测试计划
DailyMessageCollector.record()写入正确的 JSONL 文件read_window()跨日期边界过滤正确/summary on持久化到 JSON 并在重启后恢复/summary now管理员鉴权 + 冷却时间限制生效[daily_summary] enabled = false时不注册 APScheduler 任务