Skip to content

test: add session and memory replay consistency harness#238

Open
Audience-jmf wants to merge 8 commits into
trpc-group:mainfrom
Audience-jmf:feat/issue-89-replay-consistency
Open

test: add session and memory replay consistency harness#238
Audience-jmf wants to merge 8 commits into
trpc-group:mainfrom
Audience-jmf:feat/issue-89-replay-consistency

Conversation

@Audience-jmf

@Audience-jmf Audience-jmf commented Jul 26, 2026

Copy link
Copy Markdown

背景

本 PR 实现 #89 要求的 Session / Memory / Summary 多后端回放一致性测试框架。框架使用同一组标准化轨迹驱动不同后端,读取并规范化事件、state、memory 和 summary,再输出可定位到具体字段的差异报告。

主要改动

  • 新增可复用的 replay harness 和 10 条标准回放用例。
  • 覆盖单轮、多轮、工具调用、state 覆盖、作用域 state、memory 读写、summary 创建与更新、事件压缩及失败重试。
  • 默认比较 InMemory 与独立 SQLite 持久化后端。
  • 使用真实 RedisStorage 配合 fakeredis 提供无网络 Redis 模式,覆盖 TTL、query 和资源关闭语义。
  • 支持通过环境变量接入真实 SQL 与 Redis。
  • 输出包含 session id、event index、summary id、字段路径及两端值的 JSON 差异报告。
  • 对提交的基线报告执行稳定签名和 schema 校验,防止报告未参与测试或字段静默漂移。
  • 修复回放测试发现的 Redis 字符串解码与多字段 HSET 兼容问题,并保留 Redis Cluster 构造参数。

一致性策略

自动生成 ID、运行时间和字典字段顺序等非业务信息会被规范化。

allowed_diff 仅允许以下精确路径:

  • $.session.last_update_time
  • $.summary.summary_timestamp

事件顺序、state、memory 内容,以及 summary 的 session 归属、文本、版本和覆盖关系仍进行严格比较,不会被模糊忽略。

验收结果

指标 结果
正常回放用例 10 / 10 通过
人为注入不一致 10 / 10 检出
正常用例误报率 0%
注入问题检出率 100%
Summary 丢失、覆盖错误、归属错误 3 / 3 检出
默认矩阵运行耗时 约 0.54 秒
GitHub Checks 6 / 6 通过
Codecov 修改且可覆盖代码全部被测试覆盖

运行方式

# 验收测试
pytest tests/sessions/test_replay_consistency.py -q

# 仅运行 InMemory
python -m tests.sessions.replay_harness --light

# 默认运行 InMemory + SQLite 并生成报告
python -m tests.sessions.replay_harness \
  --output tests/sessions/session_memory_summary_diff_report.json

# 设置 TRPC_REPLAY_SQL_URL / TRPC_REPLAY_REDIS_URL 后运行集成模式
python -m tests.sessions.replay_harness --integration

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

我已经完成了审查。让我来定稿。

基于我对 pr.diff 的审查以及跨 SDK 源码(_base_session_service.py_redis_session_service.py_redis.py_sql_session_service.py_summarizer_manager.py_session_summarizer.py、内存服务、_ttl.pyEvent/LlmResponse)的验证,以下是我的发现。

发现的问题

🚨 Critical

  • tests/sessions/replay_harness.py:450-469create_mock_redis_backend 用进程内 _InProcessRedisStorage 替换 _redis_storage,但 Redis 会话/内存服务在构造时已经调用 _create_storage 建立真实 RedisStorage(含连接池句柄),随后被整体替换;替换后 close() 调用的是替换对象的 close()(空实现),而原 RedisStorage 的连接池并未被显式释放。更关键的是,_InProcessRedisStorage.execute_command 未实现 typelrangesmemberszrangeexpire 之外的 get/set/keys/hset/hgetall/rpush 之外的任何命令——当 SDK 内部逻辑调用未覆盖命令时会 raise ValueError。当前用例恰好只走覆盖的命令,但这是一条依赖私有内部调用顺序的脆弱路径:一旦 SDK 调用面扩展(例如 _refresh_ttl 在启用 TTL 时调用 expiresearch_memoryquery 内的 type/lrange),mock 将抛异常或行为偏离真实 Redis,使“Redis 一致性”断言失去意义。
    • 建议要么补齐 type/lrange/expire 等命令、要么直接复用真实 RedisStorage 配合 fakeredis,避免 mock 与真实实现语义漂移。

⚠️ Warning

  • tests/sessions/replay_harness.py:545-546:partial 事件“state_clean”检查只验证 attempted_state_delta 中的键值未变,但 BaseSessionService._apply_temp_state_base_session_service.py:107)会对 temp: 前缀的键写入 session.state。当前 partial_retry 的 delta 是 recovery_status(非 temp 前缀),检查成立;但若后续加入 temp: 前缀的 partial delta,_get_session 返回的过滤视图可能仍含临时态,检查逻辑不会捕获到 temp 残留,断言会比真实保证更宽松。

    • 建议在 partial 检查中显式排除/处理 temp: 前缀键,或补充一条 temp 残留断言。
  • tests/sessions/replay_harness.py:864-918run_replay_matrix 先串行跑完所有 case × backend 的快照,再做注入故障比对,且 INJECTED_FAULTSsnapshots[baseline.name][case_id] 作为期望源。这本身可接受,但 execute_case 内对每个 case 都先 delete_sessioncreate_session,在 sqlite:///:memory: 共享内存库、多 backend 并存时各 backend 独立库互不干扰;然而 test_replay_matrix_meets_acceptance 同时构造 sqlitesql_fallback 两个 同一 :memory: URL 的 SQL backend(pr.diff:2082-2083),每个 SqlStorage 各自建库,逻辑上隔离,但若未来有人改为共享 engine,会静默串数据。属于隐性耦合,建议在 backend 工厂里为每个 SQL backend 生成唯一临时库 URL 以消除该假设。

  • tests/sessions/replay_harness.py:932-935metricsfalse_positive_rate = normal_failures / len(normal_results)injected_detection_rate = ... / len(injected_results)summary_fault_detection_rate = ... / len(summary_faults) 均为除法,若 normal_results/injected_results/summary_faults 为空会 ZeroDivisionError。当前 REPLAY_CASES/INJECTED_FAULTS 非空故不会触发,但 run_replay_matrix 是公共 harness,传入空 backends 已被防御,空 case 列表却未防御。

    • 建议对分母做 len(...) or 1 保护或前置校验。

💡 Suggestion

  • tests/sessions/session_memory_summary_diff_report.json:1001:提交进仓库的报告 generated_at"2026-07-24T14:29:15Z" 硬编码时间戳,且该文件不被任何测试断言读取(测试用 tmp_path 重生成)。作为可复现基线文件保留无碍,但易随时间产生“陈旧产物”误读,建议在 README/design 中注明此为静态基线、或由 CI 比对而非直接提交运行产物。

总结

整体为新增的会话/记忆/摘要回放一致性测试套件,核心逻辑与 SDK 现有 API(create_sessionappend_eventcreate_session_summaryget_session_summarystore_session/search_memorystore_historical_events、TTL clean_ttl_config)对接正确,注入故障检测矩阵设计合理。主要风险集中在 mock Redis 实现与真实 RedisStorage 的语义覆盖缺口(Critical),以及若干边界与隐性耦合(Warning),其中 Critical 项建议修复以避免测试在 SDK 演进后给出误导性结论。

测试建议

  • 补一条用例:在 create_mock_redis_backend 场景下启用 TTL(不调用 clean_ttl_config)或触发 search_memoryRedisStorage.querytype/lrange 分支,验证 mock 不会抛 ValueError 并与真实 Redis 语义一致。
  • 补一条用例:partial 事件携带 temp: 前缀 state_delta,断言 temp 值不在持久化 session.state 中残留。

"timestamp": "<generated-summary-time>" if is_summary else event.timestamp,
"partial": event.partial,
"turn_complete": event.turn_complete,
"visible": event.visible,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mock Redis 语义覆盖缺口,断言易失真

create_mock_redis_backend 用进程内 _InProcessRedisStorage 替换已构造的真实 RedisStorage,其 close() 为空实现,原连接池未释放;且只覆盖部分命令,SDK 调用面扩展(如 TTL 的 expiresearch_memorytype/lrange)时会抛 ValueError 或偏离真实 Redis 语义,使 Redis 一致性断言失去意义。建议补齐命令或改用真实 RedisStorage + fakeredis

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@6a2f7f9). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             main        #238   +/-   ##
==========================================
  Coverage        ?   87.86812%           
==========================================
  Files           ?         482           
  Lines           ?       45162           
  Branches        ?           0           
==========================================
  Hits            ?       39683           
  Misses          ?        5479           
  Partials        ?           0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

确认了 — 提交的 JSON 从未被加载或对比过;测试只会写入 tmp_path。因此,提交的 JSON 是无效的产物。设计文档声称“仓库 JSON 是静态基线,测试会重建并校验”,这是误导性的 — 测试并不会针对仓库中的 JSON 进行校验。这是一个真正的测试有效性/文档问题。

现在让我更仔细地检查一下 duration_seconds <= 30 这个断言 — 这是一个真正的 CI 稳定性风险吗?4 个后端 × 10 个 case + 故障注入。在较慢的 CI 运行环境中,可能会超过 30 秒。但 lightweight 测试也有 30 秒的限制。这是现有的惯例。我可以将其标记为 CI 稳定性警告,但风险较低。让我看看是否有更紧凑的 SLA。实际上,更值得担心的是:test_replay_matrix_meets_acceptance 运行了 4 个后端,包括 2 个 SQL(存在磁盘 I/O)。30 秒的限制可能比较紧张,但在现代 CI 上应该没问题。我暂时不标记此项。

让我再重新考虑一个真正的问题:test_replay_matrix_meets_acceptance 断言 false_positive_rate <= 0.05。如果有 10 个正常 case,0.05 意味着最多允许 0.5 个失败 → 0 个失败。所以实际上它断言的是没有失败。<= 0.05 的写法有点奇怪(本可以是 == 0.0),但也是有效的。不算 bug。

现在让我完成最终报告。我将报告:

  • 警告:提交的 baseline JSON 从未被测试校验;main() 的默认重新生成会使用不同的后端集覆盖它 → 导致 baseline 不一致 / 具有误导性的设计文档。
  • 建议:fail_next 命名/语义(可选)。

让我再仔细检查一下 _generate_default_report 是否存在真正的问题 —— 当没有设置 TRPC_REPLAY_SQL_URL 时,它会使用 create_sqlite_backend(name="sql_fallback")。当设置了 URL 时,使用 create_sql_backend(sql_url, name="sql")。对于 redis 也是类似。没问题。

等等,这里有一个真正的问题:在 _generate_default_report 中,当 include_integrations=True 且设置了 TRPC_REPLAY_SQL_URL 时,它会调用 create_sql_backend(sql_url, name="sql")。但 create_sql_backend 没有清理资源(没有 TemporaryDirectory)。而且 BackendBundle.close 会调用 session_service.close()。对于真实数据库,这是没问题的。不是 bug。

让我重新考虑一下是否真的存在任何严重(Critical)问题。我认为没有安全/逻辑/数据方面的 bug。让我写下总结。

其实,让我再重新考虑一个潜在的实际 bug:在 run_replay_matrix 中,baseline = backends[0]。对于 test_replay_matrix_meets_acceptance,baseline = in_memory。注入的故障比较使用 snapshots[baseline.name][case_id] 作为预期值,并与 corrupted = copy.deepcopy(expected) 进行对比。所以注入的故障仅在 baseline (in_memory) 上进行测试。故障检测是在 in_memory 上通过 compare_snapshots 测量的,而不是在所有后端上。这是没问题的 — 注入测试旨在验证 diff 工具能否检测到损坏,而不是针对每个后端。设计文档符合这一点。

好的,让我完成总结。让我重新考虑一下 SQL 中 time_diff > 1.0 的过期重载(stale-reload)路径是否会被意外触发。在 create_session 之后,session.last_update_time = storage_session.update_timestamp_tz(真实时间)。然后是 append_eventstorage_session.update_timestamp_tz - session.last_update_timestorage_session 是重新获取的;它的 update_timestamp_tz 与创建时相同(尚未更改)。session.last_update_time = 创建时的值。差值 ≈ 0。不会触发过期重载。很好。

但是多个 append_events 之后呢?在第一次 append_event 之后,SQL 设置 session.last_update_time = storage_session.update_timestamp_tz(提交后刷新 → 新的真实时间)。然后第二次 append_event:重新获取 storage_sessionupdate_timestamp_tz = 第一次提交后的时间)。session.last_update_time = 第一次提交后的时间。差值 ≈ 0。很好,不会触发。

但测试框架在操作之间没有重新获取 session(仅在部分/摘要之后)。对于事件操作,它使用来自上一个 append_event 返回/突变的同一个 session 对象。实际上 execute_case 在每个事件操作上调用 append_event(session, event),没有重新获取。在 SQL 的 append_event 之后,session.last_update_time 被更新为存储的时间。所以下一个 append_event 使用更新后的 last_update_time。差值 ≈ 0。很好。

但是等等 — 对于 InMemory,append_event 之后,session.last_update_time = event.timestamp(来自 _add_event_and_get_filtered_events)。所以 session.last_update_time = BASE_TIMESTAMP + seq。然后是下一个事件。InMemory 没有过期检查。没问题。

对于 SQL,session.last_update_time 在每次 append_event 之后被设置为存储的真实时间。所以它保持为真实时间。没有过期重载。很好。

现在,BASE_TIMESTAMP = 4102444800.0(2100 年)的注释说“避免了意外触发 SQL 的过期写入恢复分支”。但正如我所展示的,last_update_time 跟踪的是存储的真实时间,而不是 BASE_TIMESTAMP。所以该注释的原理是错误的 — BASE_TIMESTAMP 不影响过期检查(它比较的是 storage_session.update_timestamp_tzsession.last_update_time,两者都是真实时间)。事件时间戳(BASE_TIMESTAMP+seq)仅存储在 SessionStorageEvent.timestamp 中,而不在 update_timestamp_tz 中。所以该注释具有误导性,但不是 bug。只是一个说明。

嗯,其实等一下 — 让我重新考虑。事件上的 SQL TTL 过滤:_apply_event_filtering_and_get_filtered_events 使用 cutoff_time = time.time() - event_ttl_seconds 并比较 e.timestamp >= cutoff_time。事件时间戳 = BASE_TIMESTAMP + seq = 2100 年。time.time() = 2026 年。所以 event.timestamp (2100) >= cutoff (2026 - ttl)。始终为真 → 事件永远不会被 TTL 过滤。所以 BASE_TIMESTAMP(未来)确保了事件不会被 TTL 过滤掉。这就是其真正的目的!注释说“避免了意外触发 SQL 的过期写入恢复分支”,但实际效果是避免了事件 TTL 过滤。测试配置禁用了 TTL (clean_ttl_config),所以 event_ttl_seconds=0 → 不会应用 TTL 过滤。所以 BASE_TIMESTAMP 与 TTL 过滤无关。

那么 BASE_TIMESTAMP 到底有什么作用呢?它会设置 session.last_update_time = event.timestamp(仅在 InMemory/Redis 中,因为 SQL 使用存储的真实时间)。对于 InMemory,last_update_time = BASE_TIMESTAMP+seq。该值包含在快照中,但 $.session.last_update_time 是允许的差异项。所以 BASE_TIMESTAMP 没有实际作用。注释具有误导性,但属于美观问题。不是可报告项。

好了。让我完成总结。我将报告关于 JSON baseline 问题的警告以及一个小建议。

让我再重新考虑一下是否应该将 JSON baseline 问题标记为警告。它确实存在:一个提交的产物没有被测试,设计文档声称它已被校验,而 main() 会用不同的内容覆盖它。这是一个真正的测试有效性 + 可维护性问题。值得作为警告提出。

让我再重新考虑一下 duration_seconds <= 30 作为 CI 稳定性警告 — 我决定将其合并或跳过,因为它符合现有惯例且风险较低。跳过。

让我重新考虑一下实际的测试有效性差距:test_in_memory_lightweight_mode 断言 time.perf_counter() - started_at <= 30 以及矩阵测试的 duration_seconds <= 30。在过载的 CI 运行器上,这些可能会不稳定地失败。但这只是测试 SLA,并不是新问题。我将其作为低优先级 CI 稳定性警告提出……实际上,我还是只保留最重要的发现,以遵守“不要为了完整性而补充低价值建议”的指示。

让我完成总结。

其实,还有一个真正的问题我想验证一下:test_replay_matrix_meets_acceptance 创建了 4 个后端,其中 create_sqlite_backend()(默认名称 "sqlite")和 create_sqlite_backend(name="sql_fallback")。但两个 SQL 后端都使用 is_async=False 和同步 SQLAlchemy。它们会共享全局状态吗?不会,每个都有独立的引擎和临时数据库。很好。

但这里有一个真正的问题:两个 SQL 后端都创建了一个 SummarizerSessionManager,其 set_session_service 指向各自的服务。摘要缓存是按管理器划分的。很好,没有交叉污染。

好了,让我写总结。

让我重新考虑一下是否真的有任何严重(Critical)问题 — 没有。我将跳过“严重”部分。

让我更精准地重新考虑 JSON 基线警告 — 有两个不同的子问题:
(a) 提交的 session_memory_summary_diff_report.json 没有被任何测试加载/校验(测试会重新生成到 tmp_path 中)。因此它可能悄无声息地偏离实际行为。
(b) main() 默认模式生成的是 2 个后端的报告,而提交的基线有 4 个后端;运行 python replay_harness.py 会静默地将提交的文件覆盖为不一致的状态。

这些问题具有相同的根本原因(提交的 JSON 具有误导性/未经验证),因此合并为一条警告。

还有一件事 — 让我验证一下 (b) — main() 默认情况:lightweight=False(未传入 --light),include_integrations=False。所以 backends = [in_memory, sqlite](2 个后端)。报告已写入默认输出路径 = 仓库中提交的 JSON。所以运行 python replay_harness.py 会用 2 个后端的报告覆盖 4 个后端的提交文件。确认 (b) 成立。

现在让我写总结。我已对 pr.diff(5 个新增文件,均在 tests/sessions/ 下)以及相关 SDK 上下文(_in_memory_session_service_sql_session_service_redis_session_service_redis.py_summarizer_manager_session_summarizer_utils.py)进行了审查。测试框架(harness)连贯,且 mock Redis 客户端与 RedisStoragequery/execute_command/expire 调度保持一致。这是我的审查结论。

发现的问题

⚠️ Warning

  • tests/sessions/session_memory_summary_diff_report.json:1(附近行):提交的基线报告从未被测试校验,且可被 main() 静默覆盖为不一致内容

    • test_replay_consistency.py:41-43 只把报告写到 tmp_path 并断言其内容,从不读取仓库中已提交的 session_memory_summary_diff_report.json;而 replay_consistency_design.md:3 声称“仓库 JSON 是静态基线,测试会重建并校验”,实际并未校验。更严重的是 replay_harness.py:934-955main() 默认模式(lightweight=False, include_integrations=False)只跑 in_memory + sqlite 两个后端(见 _generate_default_report replay_harness.py:920-922),而默认 --output 正是仓库内这份 JSON(replay_harness.py:936-939),与已提交报告的 4 后端(in_memory/sqlite/sql_fallback/redis_mock)不一致。任何人执行 python replay_harness.py 都会用 2 后端版本覆盖 4 后端基线,且无测试能发现漂移。建议:要么新增测试用 git / 文件比对重建报告与提交基线一致(至少断言 backends/case_count/metrics),要么让 main() 默认输出到非提交路径,并修正设计文档表述。
  • tests/sessions/replay_harness.py:67-72DeterministicSummaryModel.fail_next 实际不产生失败,"失败恢复"用例并未走失败路径

    • fail_next=Truegenerate_async 直接 return(空异步生成器),_session_summarizer._compress_session_to_summary(_session_summarizer.py:331-345) 正常结束得到空 summary,随后 create_session_summary_by_eventsif summary_text: 为假跳过写入(_session_summarizer.py:390)。也就是说 summarize_failure 用例(replay_cases.py summary_update) 校验的是"空摘要不写入",而非真正的异常/失败恢复路径。当前能通过是因为空摘要与异常都被归一为 summary_text="",但若日后 summarizer 对空摘要改用不同处理(如重试或报错),failed_summary_preserves_session/failed_summary_preserves_summary 将不再覆盖预期恢复语义。建议让 fail_next 抛异常以真实模拟失败,或显式重命名并在注释中说明其语义。

💡 Suggestion

  • tests/sessions/replay_harness.py:52-55BASE_TIMESTAMP 的注释与实际作用不符。BASE_TIMESTAMP + sequence 只被写入 Event.timestamp,而 SQL 的 stale-writer 判断(_sql_session_service.py:551 time_diff = storage_session.update_timestamp_tz - session.last_update_time)比较的是存储提交时钟与 session.last_update_time,两者都是真实时间,不受 BASE_TIMESTAMP 影响;该常量真正的作用是让事件时间戳远大于 time.time() 以避开事件 TTL 过滤(但测试已通过 clean_ttl_config 关闭 TTL)。建议修正注释,避免误导后续维护者。

总结

整体风险较低,未发现安全或核心逻辑层面的阻塞问题;mock Redis 实现与真实 RedisStorage 的命令/查询/TTL 调度保持一致,注入式故障检测矩阵覆盖完整。主要问题集中在测试有效性:提交的基线 JSON 既未被校验又会被 main() 默认输出覆盖,且 fail_next 并未真正模拟失败路径,建议修复 Warning 项以保证回放基线的可信度。

测试建议

  • 新增一条测试:运行 run_replay_matrix 重建报告后,与仓库中已提交的 session_memory_summary_diff_report.json 比较 backendscase_countmetrics 及各 normal_cases/injected_cases 的结构(时间戳类字段除外),以锁死基线、防止 main() 误覆盖。
  • summarize_failure 路径补一个真正抛异常的模型分支测试,验证 summarizer 在异常情况下仍保留旧摘要与会话状态。

@Audience-jmf

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

Rook1ex added a commit to trpc-group/cla-database that referenced this pull request Jul 26, 2026
@Audience-jmf
Audience-jmf force-pushed the feat/issue-89-replay-consistency branch from bf1a456 to d426919 Compare July 26, 2026 07:47
@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

现在让我更仔细地重新检查一下 event_order 的错误检测。_swap_first_events 交换了 events[0]events[1]。提交的报告显示 event_order 被检测到有许多差异。但等一下 —— compare_snapshots 是按列表顺序进行比较的。交换后,索引 0 处现在有了原来索引 1 的内容,反之亦然。因此会产生差异。被检测到。很好。

但有一个值得注意的细微问题:event_order 错误仅在顺序交换导致位置不匹配时才会被检测到。如果两个事件完全相同,交换就不会产生差异。对于 multi_turn,事件 0 和 1 是不同的(用户与助手),所以可以检测到。很好。

现在让我重新考虑一个真正的问题:在 _swap_first_events 中,如果 events 的元素少于 2 个,events[0], events[1] = events[1], events[0] 可能会失败。但这仅适用于 multi_turn,它有 6 个事件。没问题。

现在让我重新考虑 _corrupt_tool_response —— 它通过 next(...) 查找带有 function_response 的事件。对于 tool_call 用例,存在该事件。很好。但如果找不到,next 会引发 StopIteration。仅适用于结构良好的用例。没问题。

让我完成工作。我认为这是一个高质量的 PR,没有关键问题。让我找出 1-2 个真正的建议/警告。

一个真正的警告:_FakeRedisStorage.close 调用了 self._client.close(),但没有调用 super().close()。基类 RedisStorage.close 会关闭连接池。由于伪造存储从不会创建池,这是可以的。但存在不一致。不算实质性内容。

让我重新考虑最具体的问题:DeterministicSummaryModel.generate_async 是一个 async def,其中使用了 yield —— 是一个异步生成器(async generator function)。摘要器执行了 async for llm_response in self._model.generate_async(...)。很好。但如果 fail_next=True 时,它在 yield 之前就引发了异常呢?摘要器捕获异常并返回 ""。failure_count 在引发之前已经递增。很好。

让我再核实一下 partial_retry 用例的 turn_complete 和重试语义没有问题。第二个助手事件 (retry-event) 不是部分的,turn_complete=Trueappend_event 会持久化它。但第一个部分事件具有相同的 event_id="retry-event"。持久化后,session.events 包含一次 retry-event。测试断言 count("retry-event")==1。很好。

现在进行最终总结。让我重新考虑关于 canonical_reportnormalize_dynamic_value 有一个实际值得注意的局限性:它仅规范化 $.session$.summary 处的 last_update_timesummary_timestamp。但允许的差异路径 $.summary.summary_timestamp 是一个叶子节点路径。在 canonical_report 中,叶子节点差异路径在 dynamic_paths 中 → 值被替换为 "<allowed-value>"。此外,normalize_dynamic_value("$.summary", {...}) 也会替换对象级别的 summary_timestamp。双重覆盖,没问题。

我已经完成了彻底的审查。让我写下结论。

发现的问题

⚠️ Warning

  • trpc_agent_sdk/sessions/_redis_session_service.py:97 / trpc_agent_sdk/memory/_redis_memory_service.py:54_create_storage 新增 decode_responses=True 默认值,改变了 Redis 返回值类型

    • 此默认值会使 hgetall/get 等命令返回 str 而非 bytes,属于对公开 service 工厂方法的行为变更。虽然 SDK 内部消费方(model_validate_json、state dict 合并)确实按字符串语义工作、方向正确,但任何继承 RedisSessionService/RedisMemoryService 且未显式传 decode_responses、并依赖 bytes 返回的下游子类或自定义消费代码会受影响。建议在 changelog/release notes 中明确该默认值变更;所幸用的是 setdefault,显式传 decode_responses=False 的调用仍被尊重,不构成硬性破坏。
  • tests/sessions/replay_harness.py:534-536create_mock_redis_backendpatch(...RedisStorage, side_effect=storage_factory) 注入 fakeredis

    • side_effect 工厂每次调用都会新建一个 FakeRedis 客户端(session 与 memory 各一份),二者通过共享 FakeServer 协作,逻辑正确;但 storage_factory 忽略所有传入参数(*_args, **_kwargs),若未来 RedisSessionService/RedisMemoryServiceRedisStorage 构造传递额外关键参数,该 mock 会静默丢弃,差异不易被发现。建议在工厂内对关键 kwargs(如 decode_responses)做断言或显式记录被忽略,避免 mock 与真实构造签名漂移。

💡 Suggestion

  • tests/sessions/replay_harness.py:328-346_FakeRedisStorage 重写 close 只关闭注入的 client、未调用 super().close(),且 create_redis_engine 被空实现绕开 _redis_pool。当前因 fakeredis 无连接池而正确,但与基类语义有偏差;可在注释中已说明的前提下,补一句“刻意不创建 pool”,便于后续维护者理解为何不调用 super。

总结

整体风险较低,未发现必须修复的阻塞问题。核心改动(decode_responses 默认值、variadic HSET 走 execute_command_deserialize_value 支持 str)方向正确且有对应单测与回放一致性矩阵覆盖;主要需关注 decode_responses 默认变更对下游子类的潜在行为影响,建议在发布说明中显式声明。

测试建议

  • 建议补充一个用例:显式以 decode_responses=False 构造 RedisSessionService/RedisMemoryService,验证 setdefault 确实尊重调用方显式传入值(防止默认值被误改为强制覆盖)。
  • 建议在回放矩阵中增加单 field 的 HSET 用例(len(args)==3),覆盖仍走 conn.hset 单对辅助方法、不走 raw execute_command 的分支,避免该分支回归未被检测。

@helloopenworld

Copy link
Copy Markdown
Contributor

AI Code Review

好的。我已经有足够的信息来撰写审查意见了。让我仔细检查一下剩余的顾虑 —— _check_failures 验收测试只检查快照检查(snapshot checks),而注入故障(injected faults)使用的是基线(内存)快照。这个设计逻辑看起来很连贯。让我来整理一下审查结论。

发现的问题

🚨 Critical

  • tests/sessions/test_replay_consistency.py:70
    • 提交基线报告对比依赖易变的真实运行时间,CI 上必然失败
    • canonical_report(persisted_report) == canonical_report(committed_report) 把本次 pytest 运行产生的报告与仓库中提交的 session_memory_summary_diff_report.json 做全等比较。canonical_report 只归一化了 generated_atduration_seconds 以及两条 ALLOWED_DIFFS 路径($.session.last_update_time$.summary.summary_timestamp)的值,但报告中所有非 allowed 差异的 left_value/right_value 都被原样保留。任何依赖运行环境/时钟/SQLite 文件路径的差异(例如 SQLite 持久化后的浮点精度、事件排序差异、不同机器上的临时目录相关字段)都会让等式不成立而使 CI 失败。修复方向:要么把整份提交基线也纳入归一化(仅比较结构/字段集合而非具体值),要么在 CI 中仅断言指标和结构、不做整报告全等比较。
    • 相关片段:
      assert canonical_report(persisted_report) == canonical_report(committed_report)

⚠️ Warning

  • trpc_agent_sdk/storage/_redis.py:275HSET 多字段强制走 execute_command 后丢失了 command.expire 之外不受影响,但单字段路径仍走 method(*args)——redis-py 的 hset 在传入 (key, field, value) 时返回的是新增字段数,而多字段 execute_command("HSET", ...) 返回值语义一致;但 use_raw_hset 的判定 len(command.args) > 3 and not command.kwargs 会把“恰好两字段(4 个 args)”以上全部走原生命令,这在 decode_responses=Trueexecute_command 的返回值由 redis-py 按编码解码,与 hset helper 一致,问题不大。真正风险在于:调用方 _update_app_state/_update_user_state 构造的 hset 永远是 args=(key, k1, v1, k2, v2, ...) 多字段,因此单字段 helper 分支几乎不被生产路径覆盖,新增的两个单测(test_execute_command_*hset)覆盖了 helper 与 raw 两条路径但未覆盖“多字段 + TTL expire”组合下的 key 派生(command.expire.key = command.args[0])在 raw 分支同样生效。建议补一条多字段 hsetexpire 的断言,确认 TTL 仍绑定到 args[0]

  • tests/sessions/replay_harness.py:347-358_FakeRedisStorage.close() 调用 await super().close(),而基类 RedisStorage.close()self._redis_poolNone 时直接 return(fake 不创建池),因此 super().close() 是空操作——注释称“keeping its close contract safe if pool setup changes”,但 self._client.close() 是同步 fakeredis 关闭,若将来 _is_async 切到异步会漏关。当前仅同步路径安全;建议显式断言 is_async=False 或对异步 client 也做处理,避免后续误用。

  • tests/sessions/replay_harness.py:418-432summarize_failure 分支):DeterministicSummaryModel.generate_asyncfail_next=True 时抛 RuntimeError,但 SessionSummarizer._generate_summary/create_session_summary_by_events 都用 except Exception 吞掉异常并返回 None/原 events,因此 create_session_summary 不会向上抛错。checks["summary_model_failed"] 断言 failure_count == failures_before + 1 依赖模型确实被调用——但 should_summarize_sessionsummary_update 用例中“later 3 轮对话”后是否触发摘要需 check_summarizer_functions=[lambda _session: True],这总为真,OK;但若 events 不足 keep_recent_count 会被 find_events_for_summary 提前返回 None 而不调用模型,failure_count 不增,断言会失败。当前 summary_update 用例先写 v1(4 轮)后 3 轮再失败,events 数量足够,暂不触发;但这是脆弱的隐式依赖,建议在用例注释中固定 events 数量下限或显式 force=True

💡 Suggestion

  • tests/sessions/replay_harness.py:1163-1164_generate_default_reportawait create_sql_backend(sql_url, name="sql") if sql_url else await create_sqlite_backend(name="sql_fallback") 与 redis 同样的三元表达式可读性差,且 --integration 模式下两个 fallback 后端名固定为 sql_fallback/redis_mock,与 test_sql_integration_or_sqlite_fallback 的断言耦合;建议抽成显式 if/else 以降低维护成本。

总结

存在一个 Critical 问题:验收测试用整报告全等比较提交基线,依赖运行环境与时钟,CI 上极易失败,必须修复。其余为测试 harness 的边界与隐式依赖问题,建议一并加固。整体 diff 的生产代码改动(decode_responses 默认值下沉到 _create_storage_deserialize_value 支持 str、多字段 HSET 走原生命令)逻辑正确且与既有调用方一致。

测试建议

  • 补一条多字段 hsetargs 长度 > 3)携带 expire 的用例,断言 command.expire.key 被正确派生为 args[0] 且 TTL 生效,覆盖 raw HSET 分支与 expire 联动。
  • canonical_report 增加一条断言:在两份仅 generated_at/duration_seconds/allowed 路径值不同、其余完全相同的报告上等价,验证归一化范围,避免后续扩 ALLOWED_DIFFS 时回归到当前的全等比较脆弱点。

assert all(
any(difference["summary_id"] for difference in result["differences"])
for result in persisted_report["injected_cases"] if result["fault_id"].startswith("summary_"))
assert canonical_report(persisted_report) == canonical_report(committed_report)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

提交基线报告全等比较依赖易变运行环境,CI 必失败

canonical_report(persisted_report) == canonical_report(committed_report) 把本次运行产生的报告与仓库提交基线做全等比较,但归一化只覆盖 generated_at、duration_seconds 及两条 allowed 路径,其余 left_value/right_value 原样保留。任何依赖时钟/SQLite 精度/临时目录的差异都会让等式不成立。建议仅断言指标与结构,或把整份基线纳入归一化。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants