feat(session): 新增多后端 replay 一致性测试框架（#2001）#2242
Conversation
新增 session/replaytest 包，支持 Session/Memory/Summary/Track 在不同后端上按相同步骤重放，并做规范化对比与故障注入校验。 Fixes trpc-group#2001
📝 WalkthroughEnglishChange overview
Compatibility and behavioral risks
Recommended validationgo test ./session/replaytest
cd session/replaytest/sqlite && go test .
中文变更概览
兼容性与行为风险
推荐验证步骤go test ./session/replaytest
cd session/replaytest/sqlite && go test .
WalkthroughAdds ChangesReplay contracts, cases, and deterministic fixtures
Snapshot normalization and comparison
Replay execution and SQLite backend
Fault detection and JSON reporting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
|
||
| // Single-backend self-check: always pass when only one backend executed. | ||
| if len(snaps) == 1 { | ||
| cr.Status = StatusPassed |
There was a problem hiding this comment.
A capability skipped case with one runnable backend is reported as passed, so the report overstates coverage. Keep StatusSkipped when len(snaps) == 1 after any backend was skipped.
中文
只有一个后端可运行的能力跳过用例会被报告为通过，报告会高估覆盖。存在后端跳过时，`len(snaps) == 1` 仍保持 `StatusSkipped`。
|
|
||
| require ( | ||
| github.com/mattn/go-sqlite3 v1.14.32 | ||
| trpc.group/trpc-go/trpc-agent-go v1.6.1-0.20260311094958-7b74ee59e339 |
There was a problem hiding this comment.
The sqlite module requires an older root version, so external builds miss session/replaytest. Pin the root requirement to the revision that adds this package.
中文
sqlite 模块依赖旧的根模块版本，外部构建会缺少 `session/replaytest`。请将根模块依赖固定到新增该包的版本。![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
session/replaytest/DESIGN.md (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Add a language identifier (e.g.,
text) to the fenced code block to resolve the markdownlint warning and improve formatting.中文
为代码块指定语言。
为代码块添加语言标识符（例如
text），以解决 markdownlint 警告并改善格式。♻️ Proposed fix
-``` +```text ReplayCase.Steps -> execute on each NamedBackend -> Snapshot -> Normalizer (IDs/timestamps/private keys/memory hash) -> Comparator (+ AllowedDiff) -> Report JSON</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@session/replaytest/DESIGN.mdaround lines 23 - 30, Update the fenced code
block containing the ReplayCase.Steps flow in DESIGN.md to include a text
language identifier, preserving the block’s existing content and formatting.</details> <!-- cr-comment:v1:5d9364a6563c903faea868de --> _Sources: Path instructions, Linters/SAST tools_ </blockquote></details> <details> <summary>session/replaytest/harness.go (1)</summary><blockquote> `26-42`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **`HarnessOpts.Mode` is write-only w.r.t. behavior — only feeds report metadata.** `opts.Mode` is defaulted here and later only used to populate `Report.Mode` (in `report.go`). It doesn't gate case selection, per-case timeouts, or a lightweight-vs-integration run budget. The PR's stated lightweight-mode ≤30s requirement isn't enforced by this field anywhere visible in this batch — the sqlite integration test instead manually hand-picks a case subset (`sqlite/factory_test.go`) rather than relying on `Mode`. Worth confirming whether `Mode` is meant to actually control behavior (e.g., in `cases.go`, not in this batch) or is intentionally left as a caller-driven convention/report label only, to avoid future maintainers assuming it gates timing/case selection. <details> <summary>中文</summary> `opts.Mode` 在此处被赋予默认值，之后仅用于填充 `Report.Mode`（在 `report.go` 中），并不会门控用例选择、单用例超时或轻量/集成模式的运行预算。PR 中所述的轻量模式 ≤30 秒的要求，在本批次可见范围内并未通过该字段被强制执行——sqlite 集成测试（`sqlite/factory_test.go`）反而是手动挑选用例子集，而非依赖 `Mode`。建议确认 `Mode` 是否本应真正控制行为（例如在未纳入本批次的 `cases.go` 中），还是有意仅作为调用方约定/报告标签，以避免后续维护者误以为它会门控时间预算/用例选择。 </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@session/replaytest/harness.goaround lines 26 - 42, Confirm the intended
semantics of HarnessOpts.Mode across NewHarness and the harness execution flow:
either make it control lightweight behavior such as case selection and time
budgets, including enforcement of the stated 30-second limit, or explicitly keep
it as caller-defined report metadata and avoid implying otherwise. Align the
relevant execution logic and sqlite integration usage with that decision, rather
than only defaulting Mode in NewHarness.</details> <!-- cr-comment:v1:25184c073917999b4ab5d938 --> </blockquote></details> <details> <summary>session/replaytest/summarizer.go (1)</summary><blockquote> `110-115`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Avoid slicing UTF-8 strings by bytes.** In Go, `len(s)` and `s[:n]` operate on bytes. If the string contains multi-byte UTF-8 characters (like Chinese characters), slicing by bytes might cut the string in the middle of a character, resulting in invalid UTF-8. Although this is test helper code, invalid UTF-8 can cause issues with JSON serialization and diff reporting. Consider casting to `[]rune` before slicing. <details><summary>中文</summary> **避免直接按字节截断 UTF-8 字符串。** 在 Go 语言中，`len(s)` 和 `s[:n]` 是按字节操作的。如果字符串包含多字节的 UTF-8 字符（如中文字符），按字节截断可能会在字符中间切断，导致非法的 UTF-8 字符串。虽然这是测试辅助代码，但非法的 UTF-8 可能会导致 JSON 序列化或差异对比报告出现问题。建议在截断前将其转换为 `[]rune`。 </details> <details> <summary>💻 Proposed fix</summary> ```diff func truncateText(s string, n int) string { - if len(s) <= n { + rs := []rune(s) + if len(rs) <= n { return s } - return s[:n] + "..." + return string(rs[:n]) + "..." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/summarizer.go` around lines 110 - 115, Update truncateText to measure and slice Unicode characters rather than UTF-8 bytes by converting the input to []rune before applying the length check and truncation. Preserve the existing return value for text within the limit and the "..." suffix for truncated text, while ensuring the result remains valid UTF-8.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@session/replaytest/comparator_test.go`:
- Around line 82-85: Update the comparator tests around Compare and the
aggregate >= 2 assertion to verify each expected diff individually: assert the
exact path, before/after values, and Allowed flag for the intended differences,
including the required track difference. Retain the existing allowed-diff
behavior while ensuring the tests fail when an expected difference is missing or
replaced by an unrelated one.
In `@session/replaytest/comparator.go`:
- Around line 556-560: Update ruleApplies to accept only the explicit RuleIgnore
value, removing the Rule == "" case from the ignore branch. During harness
initialization or validation, reject AllowedDiff entries whose Rule is missing
or not a recognized rule value, before discrepancy matching begins.
- Around line 75-77: Replace the hard-coded `tc.Name ==
"concurrent_interleaved"` branch with an explicit comparison mode on
`ReplayCase`. In that mode, align events by logical ID and reuse the normal
semantic comparisons for content, tool calls, and state deltas, relaxing only
global interleaving while preserving branch-local order.
- Around line 67-146: Extend the comparator’s event comparison and related
comparison routines, including compareBranchLocal and snapshot/track/memory
checks, to compare all observable fields retained by normalization:
event/response timestamps and extensions, summary topics/boundaries/timestamps,
track timestamps, memory participants/timestamps, and Snapshot.Errors. Record
mismatches with precise paths and use AllowedDiff for intentionally permitted
backend variance, while preserving existing comparisons.
In `@session/replaytest/faultinject.go`:
- Around line 93-103: Update the fallback branch in the snapshot summary
selection to collect and sort the keys before choosing one, then delete and use
the deterministically selected summary entry. Preserve the existing preference
for the empty key and avoid arbitrary map iteration when it is absent.
- Around line 70-74: Update the FaultDropSummary branch and the corresponding
FaultDropTrack and FaultDropMemory branches in the fault injection handler to
return an error when their target collections are empty or nil, before mutating
them. Preserve the existing drop behavior for non-empty summaries, tracks, and
memories, matching the event-drop validation.
In `@session/replaytest/harness.go`:
- Around line 109-113: Preserve an existing StatusSkipped result in the
single-backend fallback: update the len(snaps) == 1 branch so it assigns
StatusPassed only when cr.Status is not already StatusSkipped. Keep returning
the existing cr, nil result and retain passed behavior for non-skipped
single-backend runs.
- Around line 180-187: Make fallback selection in the session snapshot
initialization block deterministic: replace the range-and-break over ex.sessions
with an explicit, stable key selection, or return an error when multiple
sessions exist. Preserve captureSession behavior for the single-session case and
avoid arbitrary replay selection.
In `@session/replaytest/normalizer_test.go`:
- Around line 42-44: Update the UTC assertion for
out.Session.Events[0].Timestamp to validate its location or zero-offset zone
directly, rather than comparing the timestamp with its UTC conversion via
Time.Equal; preserve the existing failure behavior.
In `@session/replaytest/normalizer.go`:
- Around line 73-80: Update the memory canonicalization logic around
memoryContent, memoryID, and the stable sort of out.Memories to use a full
semantic key: combine normalized content with normalized topics and participants
for both sorting and generated IDs. Ensure the same normalized metadata ordering
is used consistently so semantically distinct memories receive unique keys and
MemoryID locators remain unambiguous.
In `@session/replaytest/sqlite/factory.go`:
- Around line 73-84: Update Factory so the cleanup closure returned by Open is
not discarded: wrap the returned session.Service (or its Close implementation)
so invoking Close triggers cleanup while preserving the existing service
behavior and error handling. Remove the `_ = cleanup` placeholder and ensure
cleanup is executed safely without changing the replaytest.BackendFactory
signature.
- Around line 30-34: Update Open to track whether it created the temporary
directory when dir is empty, then have the cleanup closure remove that directory
with os.RemoveAll after closing services. Preserve caller-provided directories
by only removing directories created internally.
---
Nitpick comments:
In `@session/replaytest/DESIGN.md`:
- Around line 23-30: Update the fenced code block containing the
ReplayCase.Steps flow in DESIGN.md to include a text language identifier,
preserving the block’s existing content and formatting.
In `@session/replaytest/harness.go`:
- Around line 26-42: Confirm the intended semantics of HarnessOpts.Mode across
NewHarness and the harness execution flow: either make it control lightweight
behavior such as case selection and time budgets, including enforcement of the
stated 30-second limit, or explicitly keep it as caller-defined report metadata
and avoid implying otherwise. Align the relevant execution logic and sqlite
integration usage with that decision, rather than only defaulting Mode in
NewHarness.
In `@session/replaytest/summarizer.go`:
- Around line 110-115: Update truncateText to measure and slice Unicode
characters rather than UTF-8 bytes by converting the input to []rune before
applying the length check and truncation. Preserve the existing return value for
text within the limit and the "..." suffix for truncated text, while ensuring
the result remains valid UTF-8.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25000897-0c9e-449a-bea7-106d5ba0deec
⛔ Files ignored due to path filters (1)
session/replaytest/sqlite/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
session/replaytest/DESIGN.mdsession/replaytest/README.mdsession/replaytest/backend.gosession/replaytest/cases.gosession/replaytest/comparator.gosession/replaytest/comparator_test.gosession/replaytest/doc.gosession/replaytest/events.gosession/replaytest/faultinject.gosession/replaytest/faultinject_test.gosession/replaytest/harness.gosession/replaytest/harness_test.gosession/replaytest/normalizer.gosession/replaytest/normalizer_test.gosession/replaytest/options.gosession/replaytest/profile.gosession/replaytest/report.gosession/replaytest/report_test.gosession/replaytest/sqlite/README.mdsession/replaytest/sqlite/factory.gosession/replaytest/sqlite/factory_test.gosession/replaytest/sqlite/go.modsession/replaytest/steps.gosession/replaytest/summarizer.gosession/replaytest/summarizer_test.gosession/replaytest/testdata/sample_report.jsonsession/replaytest/types.go
| func ruleApplies(rule AllowedDiff, d Diff) bool { | ||
| switch rule.Rule { | ||
| case RuleIgnore, "": | ||
| return true | ||
| case RuleNotEmpty: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require an explicit allowed-difference rule.
Treating the zero value Rule == "" as ignore means an omitted field silently suppresses every matching discrepancy. Accept only RuleIgnore explicitly, and reject missing or unknown rules during harness validation.
中文
AllowedDiff 应要求显式规则。
将零值 Rule == "" 视为 ignore 会使遗漏配置静默忽略所有匹配差异。建议仅显式接受 RuleIgnore，并在 harness 初始化时拒绝缺失或未知规则。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/comparator.go` around lines 556 - 560, Update ruleApplies
to accept only the explicit RuleIgnore value, removing the Rule == "" case from
the ignore branch. During harness initialization or validation, reject
AllowedDiff entries whose Rule is missing or not a recognized rule value, before
discrepancy matching begins.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2242 +/- ##
===================================================
- Coverage 89.87004% 89.87002% -0.00003%
===================================================
Files 1145 1164 +19
Lines 199290 204255 +4965
===================================================
+ Hits 179102 183564 +4462
- Misses 12644 12942 +298
- Partials 7544 7749 +205
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- harness 单后端路径保留 StatusSkipped - AllowedDiff 空 rule 不再当作 ignore - concurrent 改用 EventCompareMode，branch_local 对齐后做语义对比 - comparator 补 summary/track/memory/errors 字段 - faultinject 空 drop 返回错误，summary rekey 确定性 - normalizer memory 语义 key；UTC 断言修复 - sqlite Factory Close 清理临时目录 Refs trpc-group#2001
|
已根据 review 反馈完成修改并推送（commit 处理说明
本地验证：
若还有遗漏点请继续指出，我跟进。 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
session/replaytest/normalizer.go (1)
295-297: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNormalize nested memory timestamps to UTC.
To ensure stable and deterministic cross-backend comparisons, the nested pointer timestamps
entry.Memory.LastUpdatedandentry.Memory.EventTimeshould also be converted to UTC, similar toCreatedAtandUpdatedAt.中文
将嵌套的 Memory 时间戳转换为 UTC。
为确保跨后端比较的稳定性和确定性，嵌套的指针时间戳
entry.Memory.LastUpdated和entry.Memory.EventTime也应转换为 UTC（与CreatedAt和UpdatedAt类似）。💡 Proposed fix
if !entry.UpdatedAt.IsZero() { entry.UpdatedAt = entry.UpdatedAt.UTC() } + if entry.Memory.LastUpdated != nil { + t := entry.Memory.LastUpdated.UTC() + entry.Memory.LastUpdated = &t + } + if entry.Memory.EventTime != nil { + t := entry.Memory.EventTime.UTC() + entry.Memory.EventTime = &t + } // Stable semantic ID so backends with random IDs still compare.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/normalizer.go` around lines 295 - 297, Update the timestamp normalization logic around entry.UpdatedAt to also handle the nested entry.Memory pointer when present: convert Memory.LastUpdated and Memory.EventTime to UTC only when each timestamp is non-zero. Preserve the existing top-level timestamp normalization and avoid dereferencing a nil Memory pointer.Source: Path instructions
session/replaytest/comparator_test.go (1)
17-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for the remaining comparator paths.
This test only covers event content and summary-key diffs. Add one case per remaining path the comparator emits — event timestamp, event extensions, response timestamp, summary topics, summary
updated_at, summary boundary, memory timestamps, and snapshot errors — and assert the exactPath/Allowedstate for each.中文
为剩余的 comparator 路径补充表驱动覆盖。
这个测试目前只覆盖了 event content 和 summary-key 差异。请把 comparator 其余会产出的路径也各加一例——event timestamp、event extensions、response timestamp、summary topics、summary
updated_at、summary boundary、memory timestamps 和 snapshot errors——并断言每条记录的Path/Allowed状态。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/comparator_test.go` around lines 17 - 67, Extend TestComparator_DetectsEventAndSummaryDiffs with table-driven cases covering event timestamps, event extensions, response timestamps, summary topics, summary updated_at, summary boundary, memory timestamps, and snapshot errors. For each case, construct the minimal differing snapshots, run Comparator.Compare, and assert the exact emitted Path and Allowed value, while preserving the existing content and summary-key coverage.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@session/replaytest/comparator.go`:
- Around line 398-424: Extend the comparison logic around the existing event and
memory checks to explicitly compare all persisted semantics: event invocation
metadata, tags, completion and long-running markers, actions, filter keys,
versions, and every response choice; also compare memory ID, Kind, EventTime,
Location, and LastUpdated. Route each intentional backend-specific difference
through AllowedDiff, while preserving event flow, long-running markers, and
persistence semantics; keep MemoryID as report metadata only.
- Around line 501-507: Update the comparison logic around indexA and indexB to
preserve and compare every occurrence of duplicate event IDs, using
occurrence-aware keys or per-ID slices rather than overwriting map entries.
Ensure concurrent replay comparisons validate event order and semantic
consistency for each occurrence, and add a regression test covering duplicate
IDs with an earlier mismatch.
---
Nitpick comments:
In `@session/replaytest/comparator_test.go`:
- Around line 17-67: Extend TestComparator_DetectsEventAndSummaryDiffs with
table-driven cases covering event timestamps, event extensions, response
timestamps, summary topics, summary updated_at, summary boundary, memory
timestamps, and snapshot errors. For each case, construct the minimal differing
snapshots, run Comparator.Compare, and assert the exact emitted Path and Allowed
value, while preserving the existing content and summary-key coverage.
In `@session/replaytest/normalizer.go`:
- Around line 295-297: Update the timestamp normalization logic around
entry.UpdatedAt to also handle the nested entry.Memory pointer when present:
convert Memory.LastUpdated and Memory.EventTime to UTC only when each timestamp
is non-zero. Preserve the existing top-level timestamp normalization and avoid
dereferencing a nil Memory pointer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 740b4441-4174-4823-816c-5c7305f309e4
📒 Files selected for processing (13)
session/replaytest/DESIGN.mdsession/replaytest/cases.gosession/replaytest/comparator.gosession/replaytest/comparator_test.gosession/replaytest/faultinject.gosession/replaytest/faultinject_test.gosession/replaytest/harness.gosession/replaytest/harness_test.gosession/replaytest/normalizer.gosession/replaytest/normalizer_test.gosession/replaytest/sqlite/factory.gosession/replaytest/sqlite/factory_test.gosession/replaytest/types.go
💤 Files with no reviewable changes (1)
- session/replaytest/sqlite/factory_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- session/replaytest/harness_test.go
- session/replaytest/sqlite/factory.go
- session/replaytest/faultinject_test.go
- session/replaytest/DESIGN.md
- session/replaytest/cases.go
- session/replaytest/normalizer_test.go
- session/replaytest/faultinject.go
- session/replaytest/types.go
- session/replaytest/harness.go
Refs trpc-group#2001 - comparator 补全 event 持久化语义字段（invocation/tag/completion/long-running/actions/filter_key/version 及全部 response choices） - memory 显式比较 id/kind/location/event_time/last_updated - branch_local 按 (id, occurrence) 保留重复逻辑 ID，避免早先语义差异被覆盖 - normalizer 将 Memory.LastUpdated/EventTime 规范到 UTC - harness 拒绝空/未知 AllowedDiff.Rule - 增加重复 ID 与字段覆盖回归测试；DESIGN.md 代码块补 language tag
|
已处理 CodeRabbit 在 处理说明
本地验证：
若还有遗漏请继续指出。 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 6 inline comments below. The comments focus on issues worth addressing before merge.
Reviewed the replay executor, normalization, comparison, fault injection, and SQLite coverage. Six high-confidence issues remain: generated memory timestamps cause false failures; valid state and response fields can be silently ignored; duplicate backend names can skip comparison; and the recovery/concurrency cases do not exercise their advertised behavior.
中文
我看过这次变更的相关代码，在下面留下 6 条行内评论。评论聚焦在合并前值得处理的问题。
已审查 replay 执行器、规范化、比较、故障注入及 SQLite 覆盖。发现 6 个高置信度问题：自动生成的 memory 时间戳会造成误报；合法 state 与 response 字段可能被静默忽略；同名后端会绕过比较；恢复与并发用例也未真正覆盖其宣称的行为。
| Explanation: "memory participants mismatch", | ||
| }) | ||
| } | ||
| if !memoryTimeEqual(a[i], b[i]) { |
There was a problem hiding this comment.
P1: Normalize backend-generated memory timestamps
AddMemory assigns CreatedAt, UpdatedAt, and LastUpdated from each backend's local time.Now(), but the comparator requires exact equality. CaseMemoryWriteAndRead has no allowance for these fields, so two correct backends—including the two independent in-memory services used by TestAllCases_InMemorySelfConsistency—can fail solely because their calls occurred at different instants.
Canonicalize fixture-generated audit timestamps before comparison, or define an explicit tolerance/ignore rule while preserving semantic fields such as caller-supplied EventTime. Include CaseMemoryWriteAndRead in the SQLite dual-backend test so this path is validated.
中文
应规范化后端自动生成的 memory 时间戳
AddMemory 会由各后端分别通过 time.Now() 生成 CreatedAt、UpdatedAt 和 LastUpdated，但这里要求时间完全相等。CaseMemoryWriteAndRead 又没有为这些字段配置允许差异，因此两个行为正确的后端也可能仅因调用时刻不同而失败，包括自一致性测试中的两个独立 InMemory 实例。
建议在比较前统一处理测试生成的审计时间，或显式配置容差/忽略规则，同时保留调用方提供的 EventTime 等业务时间。还应把 CaseMemoryWriteAndRead 加入 SQLite 双后端测试，确保该路径真正被覆盖。
There was a problem hiding this comment.
Fixed. CreatedAt, UpdatedAt, and LastUpdated are canonicalized while EventTime is preserved, and the SQLite comparison now includes CaseMemoryWriteAndRead.
中文
已修复。CreatedAt、UpdatedAt 和 LastUpdated 已统一处理，EventTime 仍被保留参与比较，SQLite 对照测试也加入了 CaseMemoryWriteAndRead。
| add(fmt.Sprintf("events[%d].timestamp", index), ea.Timestamp, eb.Timestamp, "event timestamp mismatch") | ||
| } | ||
|
|
||
| // Compare every response choice (not only choices[0]). Keep first-choice |
There was a problem hiding this comment.
P1: Compare response-level semantics, not only choices
The event comparator checks response choices and Timestamp, but never checks response-level fields including Object, Model, Usage, Error, Done, and IsPartial. A persistence backend that drops an API error, completion marker, or object type would therefore pass replay consistency even though consumers observe different event behavior.
Compare a residual model.Response after removing only fields intentionally normalized elsewhere, and add a test that changes fields such as Object, Done, or Error while leaving choices unchanged.
中文
除 choices 外还需比较 response 顶层语义
事件比较目前只检查 response 的 choices 和 Timestamp，没有比较 Object、Model、Usage、Error、Done、IsPartial 等顶层字段。这样一来，持久化后端即使丢失 API 错误、完成标记或对象类型，也会通过一致性检查，但事件消费者实际看到的行为已经不同。
建议仅清除明确需要规范化的字段后，对剩余 model.Response 做比较；同时增加测试，在 choices 不变的情况下修改 Object、Done 或 Error，并断言能够产生 diff。
There was a problem hiding this comment.
Fixed. Response-level fields are compared through the residual response, with coverage for Object, Done, and Error changes while choices remain unchanged.
中文
已修复。response 顶层字段已通过 residual response 参与比较，并覆盖了 choices 不变时 Object、Done 和 Error 发生变化的场景。
| return | ||
| } | ||
| for k := range state { | ||
| if strings.HasPrefix(k, "_") { |
There was a problem hiding this comment.
P1: Do not discard every underscore-prefixed state key
session.StateMap does not reserve _ as a non-semantic prefix. The framework already uses underscore-prefixed state for observable control data, such as __trpc_agent_await_user_reply_route__ and _node_metadata; applications may also use such keys. Removing them from session, app, and user snapshots means a backend can lose or corrupt this state and still compare equal.
Replace the blanket prefix rule with an explicit allowlist of known backend-generated nondeterministic keys, or require callers to opt out through AllowedDiff. Add a regression where an underscore-prefixed key is missing on one side and must produce a state diff.
中文
不要丢弃所有下划线前缀的状态键
session.StateMap 并未规定 _ 是非业务字段前缀。框架本身已使用 __trpc_agent_await_user_reply_route__、_node_metadata 等下划线前缀键保存可观察的控制状态，业务代码也可能使用这类键。当前逻辑会同时从 session、app 和 user 快照中删除它们，导致后端丢失或写坏这些状态时仍被判定一致。
建议只忽略明确列出的后端非确定性字段，或要求调用方通过 AllowedDiff 主动声明。还应增加回归测试：一侧缺少下划线前缀键时必须产生 state diff。
There was a problem hiding this comment.
Fixed. Underscore-prefixed state keys are preserved; callers must use AllowedDiff for any backend-specific exclusions, and the regression test covers this behavior.
中文
已修复。下划线前缀状态键会被保留；后端特有字段需要由调用方通过 AllowedDiff 显式排除，相关行为已有回归测试。
| Description: "duplicate logical event append after recovery", | ||
| Steps: []Step{ | ||
| AppendEventStep{StepKey: "c11.user.1", SessionKey: key, Event: UserEvent("c11.user.1", "once")}, | ||
| AppendEventStep{StepKey: "c11.user.1.dup", SessionKey: key, Event: UserEvent("c11.user.1", "once")}, |
There was a problem hiding this comment.
P2: The recovery case does not append a duplicate logical event
Although both fixtures start with UserEvent("c11.user.1", ...), appendEvent overwrites EventLogicalKeyExtension with each step's StepKey. The second append therefore uses c11.user.1.dup, making it a distinct logical event. Both writes also use the same cached session, so no recovery or reload boundary is exercised.
Represent the logical event key separately from the step key and keep it identical for both appends. Add an explicit session reload or backend recovery step before the retry, then assert the intended event multiplicity so deduplication/recovery regressions cannot pass.
中文
恢复用例并未追加重复的逻辑事件
虽然两个 fixture 都从 UserEvent("c11.user.1", ...) 创建事件，但 appendEvent 会使用各自的 StepKey 覆盖 EventLogicalKeyExtension。因此第二次写入的逻辑键实际是 c11.user.1.dup，并不是重复事件；而且两次写入都使用同一个缓存 session，也没有经过恢复或重新加载边界。
建议将逻辑事件键与步骤键分开，并让两次写入使用完全相同的逻辑键。在重试前增加明确的 session reload 或后端恢复步骤，并断言预期的事件数量，避免去重或恢复路径回归仍然通过。
There was a problem hiding this comment.
The logical key and reload boundary are fixed. The regression test still only checks cross-backend equality; please also assert the recovered session's expected duplicate-event count so a shared executor regression cannot pass on both sides.
中文
逻辑键和 reload 边界已修复。不过当前回归测试只检查跨后端是否相等，仍建议断言恢复后 session 中预期的重复事件数量，避免执行器两侧同时出错时仍然通过。
| if err != nil { | ||
| return cr, err | ||
| } | ||
| snaps[b.Name] = norm |
There was a problem hiding this comment.
P2: Reject duplicate backend names before snapshot insertion
Snapshots and profiles are keyed only by b.Name, so registering two backends with the same name silently overwrites the first. The resulting map can contain one snapshot, after which the single-backend branch returns passed without comparing anything. This is easy to hit when comparing two configurations of the same backend profile.
Validate that backend names are non-empty and unique before execution, returning a configuration error on duplicates, or retain snapshots by registration index. Add a test proving duplicate names cannot collapse a multi-backend run into a pass.
中文
写入快照前应拒绝重复后端名称
快照和 profile 仅以 b.Name 为键，因此两个同名后端会静默覆盖，最终 map 可能只剩一个快照，随后单后端分支直接返回 passed，实际没有发生任何比较。对同一种后端的两个配置做对照时很容易触发这种情况。
建议执行前校验后端名称非空且唯一，重复时返回配置错误；或者按注册序号保存快照。还应增加测试，确保同名后端不能把多后端运行折叠成无比较的通过结果。
There was a problem hiding this comment.
Fixed. Run now rejects empty or duplicate backend names before snapshots can overwrite one another, and the duplicate-name case is covered.
中文
已修复。Run 会在生成快照前拒绝空名称或重复名称，并已有同名后端的回归测试。
| sessions: map[session.Key]*session.Session{}, | ||
| snapshot: &Snapshot{Backend: backend.Name}, | ||
| } | ||
| for _, step := range tc.Steps { |
There was a problem hiding this comment.
P2: Execute the interleaving case concurrently
Every replay step is executed serially in this loop, so CaseConcurrentInterleaved always performs the fixed sequence A1, B1, A2, B2. Branch-local comparison is exercised, but concurrent append locking, race handling, and nondeterministic global interleaving are not; those regressions would still pass.
Introduce an explicit parallel step/group with a start barrier and deterministic join/error collection, then run each branch sequentially within its own worker. Assert event count and per-branch order without using sleeps.
中文
交错用例需要真正并发执行
这里会串行执行所有 replay step，因此 CaseConcurrentInterleaved 始终只是固定的 A1、B1、A2、B2 顺序。虽然 branch-local 比较逻辑被调用了，但并发追加时的锁、竞态处理以及非确定性全局交错完全没有被覆盖，相关回归仍会通过。
建议增加明确的并行步骤组，使用启动屏障和确定性的等待/错误收集机制，每个 worker 内保持各自分支顺序。测试应断言事件总数与分支内顺序，不依赖 sleep。
There was a problem hiding this comment.
Fixed. CaseConcurrentInterleaved now runs branch-local sequences in parallel workers and joins them before capturing the session. A separate shared-executor synchronization issue is noted in the fresh review.
中文
已修复。CaseConcurrentInterleaved 现会在并行 worker 中执行各分支的顺序步骤，并在抓取 session 前完成汇合。本次新审查另行指出了共享执行器状态的同步问题。
问题/根因分析： 1. memory 的 CreatedAt/UpdatedAt/LastUpdated 由各后端 time.Now() 写入， 跨后端比较会把审计时间差当成语义不一致。 2. response 比较只覆盖 choices 等路径，顶层 Object/Model/Usage/Error/ Done/IsPartial/Created/ID 等 residual 字段未参与比较。 3. normalizer 一律删除 "_" 前缀 state，会误伤 _node_metadata、 __trpc_agent_await_user_reply_route__ 等控制键。 4. recovery_duplicate_event 用 StepKey 覆盖 logical key，两次 append 实际不是同一逻辑事件，也缺少 reload 边界。 5. 同名 backend 静默覆盖，排障困难。 6. concurrent_interleaved 仍是串行 step，无法覆盖真并发交错。 方案： 1. 审计时间戳归一为 FixedTimestamp；EventTime 仅转 UTC，保持语义比较。 2. comparator 增加 response residual 字段 DeepEqual。 3. 取消 blanket 删除 "_" state；跨后端噪声走 AllowedDiff。 4. AppendEventStep 增加 LogicalKey；新增 ReloadSessionStep； recovery case 共享 logical key 并在重复写入前 reload。 5. Run 前校验 backend 名称唯一。 6. 新增 ParallelGroupStep（barrier + 分支内串行），concurrent case 先 seed 再双分支真并发；sessions map 加锁。 主要变更： - session/replaytest/normalizer.go - session/replaytest/comparator.go - session/replaytest/steps.go - session/replaytest/harness.go - session/replaytest/cases.go - 对应单测与 sqlite/factory_test.go（纳入 CaseMemoryWriteAndRead） 测试： - go test ./session/replaytest/ -count=1 - go test ./session/replaytest/sqlite/ -count=1
|
已按 review 意见修改：
测试：
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
session/replaytest/normalizer.go (1)
288-309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
EventTimein the memory semantic identity.
EventTimeis deliberately preserved as semantic, but it is excluded frommemorySemanticKey. Memories differing only by occurrence time therefore share an ID and sort key, allowing backend-specific ordering to cause false index-based diffs. Include normalizedEventTimein the hash and add a reverse-order regression.中文
请将
EventTime纳入 Memory 语义标识。
EventTime被明确保留为业务语义字段，但memorySemanticKey未包含它。仅发生时间不同的 Memory 会得到相同 ID 和排序键，后端返回顺序不同时可能产生错误差异。请将规范化后的EventTime纳入哈希，并增加逆序回归测试。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/normalizer.go` around lines 288 - 309, Update memorySemanticKey to include the normalized Memory.EventTime in the semantic hash so memories differing only by occurrence time receive distinct IDs and sort keys. Preserve the existing UTC normalization in the normalizer, and add a regression test covering two otherwise-identical memories returned in reverse EventTime order.session/replaytest/harness_test.go (1)
50-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when replay cases are unexpectedly skipped.
The self-consistency test accepts only ten passes, and the recovery test checks only
FailedCases. AStatusSkippedregression therefore passes silently. Assert zero skipped cases, allAllCases()passed, and the recovery result is exactlyStatusPassed.中文
Replay case 意外跳过时应使测试失败。
自一致性测试只要求至少十个通过，恢复测试也仅检查
FailedCases。因此StatusSkipped回归会被静默放过。请断言跳过数为零、AllCases()全部通过，并确认恢复 case 的状态严格为StatusPassed。As per path instructions, test assertions must be strong enough to verify the intended behavior.
Also applies to: 150-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/harness_test.go` around lines 50 - 64, Strengthen the replay test assertions around h.Run, AllCases(), and the recovery result: require zero skipped cases, require every AllCases() case to pass rather than accepting a minimum pass count, and assert the recovery case status is exactly StatusPassed. Preserve failure diagnostics for individual failed cases.Source: Path instructions
session/replaytest/comparator.go (2)
69-76: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve nil-versus-empty session and memory presence.
A missing session is projected to empty collections, while nil memory entries/payloads are projected to empty helper values. A backend that loses an otherwise empty persisted object can therefore compare equal. Emit explicit presence diffs before comparing child fields.
中文
请保留 Session 和 Memory 的 nil 与空值差异。
缺失的 Session 会被投影为空集合，nil Memory 条目或 payload 也会被辅助函数投影为空值。因此后端即使丢失空的持久化对象，也可能被判定一致。请在比较子字段前显式报告存在性差异。
As per path instructions, persistence-related structures must preserve observable semantics across backends.
Also applies to: 367-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/comparator.go` around lines 69 - 76, Update the comparator around the session event extraction and the corresponding memory comparison to preserve presence semantics: report a diff when one Session is nil and the other is non-nil, and likewise when a memory entry or payload exists on only one side. Perform these presence checks before comparing child fields or projecting values through helper functions, while retaining existing comparisons for objects present on both sides.Source: Path instructions
633-725: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope duplicate occurrences by branch, not global interleaving.
Occurrence counters and semantic indexes currently use only
ID. If the same ID appears in two branches, opposite global interleavings assign different#0/#1tokens and pair the wrong events, although branch-local order is unchanged. Index by(effectiveBranch, ID, occurrence)and add a cross-branch duplicate-ID regression.中文
重复事件的 occurrence 应按分支划分，而非按全局交错划分。
当前 occurrence 计数及语义索引仅使用
ID。同一 ID 出现在两个分支时，不同的全局交错会分配不同的#0/#1，并错误配对事件，即使各分支内部顺序完全一致。请按(effectiveBranch, ID, occurrence)对齐，并增加跨分支重复 ID 回归测试。As per path instructions, concurrent replay comparisons must verify branch order and consistency while relaxing only global interleaving.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/comparator.go` around lines 633 - 725, Update the comparator’s duplicate-occurrence handling in indexByOccurrence, group, and the compareEventSemantics lookup to key occurrences by effective branch plus event ID, so `#n` tokens and semantic pairing are branch-local rather than affected by global interleaving. Preserve branch-local order and multiset validation while allowing equivalent cross-branch interleavings, and add a regression test covering the same ID duplicated across branches with differing global order.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@session/replaytest/comparator.go`:
- Around line 69-76: Update the comparator around the session event extraction
and the corresponding memory comparison to preserve presence semantics: report a
diff when one Session is nil and the other is non-nil, and likewise when a
memory entry or payload exists on only one side. Perform these presence checks
before comparing child fields or projecting values through helper functions,
while retaining existing comparisons for objects present on both sides.
- Around line 633-725: Update the comparator’s duplicate-occurrence handling in
indexByOccurrence, group, and the compareEventSemantics lookup to key
occurrences by effective branch plus event ID, so `#n` tokens and semantic pairing
are branch-local rather than affected by global interleaving. Preserve
branch-local order and multiset validation while allowing equivalent
cross-branch interleavings, and add a regression test covering the same ID
duplicated across branches with differing global order.
In `@session/replaytest/harness_test.go`:
- Around line 50-64: Strengthen the replay test assertions around h.Run,
AllCases(), and the recovery result: require zero skipped cases, require every
AllCases() case to pass rather than accepting a minimum pass count, and assert
the recovery case status is exactly StatusPassed. Preserve failure diagnostics
for individual failed cases.
In `@session/replaytest/normalizer.go`:
- Around line 288-309: Update memorySemanticKey to include the normalized
Memory.EventTime in the semantic hash so memories differing only by occurrence
time receive distinct IDs and sort keys. Preserve the existing UTC normalization
in the normalizer, and add a regression test covering two otherwise-identical
memories returned in reverse EventTime order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b63a3c35-7700-4d9b-85f3-6529d3fc22c5
📒 Files selected for processing (10)
session/replaytest/DESIGN.mdsession/replaytest/cases.gosession/replaytest/comparator.gosession/replaytest/comparator_test.gosession/replaytest/harness.gosession/replaytest/harness_test.gosession/replaytest/normalizer.gosession/replaytest/normalizer_test.gosession/replaytest/sqlite/factory_test.gosession/replaytest/steps.go
🚧 Files skipped from review as they are similar to previous changes (4)
- session/replaytest/sqlite/factory_test.go
- session/replaytest/DESIGN.md
- session/replaytest/harness.go
- session/replaytest/cases.go
根因分析： 1. lint gocyclo：InjectFault 约 43、runCase 为 22，超过阈值 20。 2. check go.mod version：sqlite 子模块里 memory/sqlite、session/sqlite 写成了 v0.0.0，CI 禁止该写法，也无法从 git tags 解析。 3. comparator 主路径过重；跨分支同 id 的 occurrence、session/memory 的 nil 与空值区分不够清楚。 方案： 1. 按 service_helper 风格拆分：主入口只编排，细节放到 helper。 2. InjectFault 只做 switch 分发；runCase 拆成采集快照、生成比较对、比较和收尾。 3. Compare 逻辑下沉到 comparator_helper：occurrence 按 (branch, id)；session/memory 区分 presence。 4. 修正 session/replaytest/sqlite/go.mod 依赖版本：去掉 v0.0.0，子模块用 v1.10.0，根模块固定到引入包时的 pseudo-version，保留 replace。 主要变更： - faultinject.go + faultinject_helper.go（新建） - harness.go + harness_helper.go（新建） - comparator.go + comparator_helper.go（新建）+ comparator_test.go - session/replaytest/sqlite/go.mod： - trpc-agent-go → v1.10.1-0.20260716054639-622a2371c148 - memory/sqlite、session/sqlite → v1.10.0 说明： - 重复后端名校验、交错真并发已在 d94420f 完成，本提交不改其行为。 - single-backend 遇 capability skip 时仍保持 StatusSkipped。 测试： - go test ./session/replaytest/ -count=1 - 本地 gocyclo：InjectFault=13，runCase=5
根因：PR patch 覆盖约 65% 低于 85% 门槛，steps/faultinject 错误路径、 harness 执行分支、AllowedDiff 规则与 sqlite factory 等新增代码测得少。 方案：按清单补纯测试，不改生产代码；表驱动覆盖 Type/Key、InjectFault 失败路径、AllPairs/finalize、env 门控后端、executeCase 全路径、 comparator/AllowedDiff 与 sqlite Open/Factory。 主要变更： - steps/backend/profile 单元测试 - coverage_extra / coverage_comparator 覆盖 harness/fault/comparator - sqlite factory_extra 覆盖 Open/Factory/NamedBackend 测试：go test ./session/replaytest/ PASS（整包 ~95%）； sqlite 子模块 CGO PASS（~63.6%，剩余多为 Open 错误分支）。
|
本轮处理了 review / CI 反馈，对应两笔提交：
处理内容
测试
提交
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
session/replaytest/profile_test.go (1)
10-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete profile-name contract.
This test only checks that names are non-empty and that Redis is exactly
"redis". A regression that gives InMemory and SQLite duplicate or incorrect names would still pass, weakening coverage for the backend-name uniqueness contract. Assert the expected names and uniqueness for all profiles.中文
覆盖完整的 profile 名称契约。
当前测试只检查名称非空以及 Redis 名称。即使 InMemory 和 SQLite 出现重复或错误名称，测试仍会通过，无法覆盖后端名称唯一性契约。建议断言所有 profile 的预期名称，并检查名称唯一性。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/profile_test.go` around lines 10 - 21, Update TestProfiles_NamesAndCaps to define the expected names for InMemoryProfile, SQLiteProfile, and RedisProfile, assert each profile’s actual Name matches its expected value, and verify all profile names are unique. Preserve the existing SupportsSessionState assertions.session/replaytest/backend_test.go (1)
22-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant
os.Unsetenv.
t.Setenvmanages test environment state and automatically restores it after the test. Mixing it with rawos.Unsetenvcircumvents the test framework's environment tracking. Furthermore, it is unnecessary becauseos.Getenvreturns an empty string for both unset variables and those explicitly set to"".中文
移除多余的
os.Unsetenv。
t.Setenv已经负责管理测试环境变量的状态，并在测试结束后自动恢复原值。将它与底层的os.Unsetenv混用会绕开测试框架的环境追踪。此外，这也是不必要的，因为os.Getenv对于未设置和明确设置为空字符串的变量均会返回空字符串。♻️ Proposed refactor
for _, k := range keys { t.Setenv(k, "") - _ = os.Unsetenv(k) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@session/replaytest/backend_test.go` around lines 22 - 25, Remove the raw os.Unsetenv call from the keys loop, leaving t.Setenv(k, "") as the sole environment update so the test framework can track and restore each variable.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@session/replaytest/comparator_helper.go`:
- Around line 202-213: Extend the track comparison logic around the event count
and payload/timestamp checks to also compare TrackEvents.Track and each
TrackEvent.Track. Record mismatches through add using the existing tracks event
path conventions, while preserving the current payload, timestamp, and length
comparisons.
- Around line 198-200: Update the track comparison branch around the okA/okB and
ta/tb checks to distinguish map key presence from non-nil track values. Treat
both keys present with nil values as equal, report key-presence mismatches using
okA and okB, and report one-nil value mismatches with accurate value-presence
booleans before returning diffs.
In `@session/replaytest/comparator.go`:
- Line 44: Update the comparison flow around compareSessionID to compare the
effective session IDs from both snapshots and emit a session.id diff when they
differ, rather than using the selected ID only as report metadata. Preserve the
existing ID selection for annotations while ensuring missing, dropped, or
rewritten IDs are reported.
In `@session/replaytest/coverage_extra_test.go`:
- Around line 532-536: Update the clone-isolation assertion in the normalization
test around out.AppState and snap.AppState so shared mutable state causes the
test to fail rather than merely logging with t.Log. Assert that mutating
out.AppState does not change snap.AppState, preserving the normalizer’s
isolation contract.
In `@session/replaytest/faultinject_helper.go`:
- Around line 75-95: Update the summary-selection logic around the empty-key and
sorted-key branches to choose a non-nil *session.Summary before mutating
snap.Session.Summaries. Prefer the empty-key entry only when non-nil; otherwise
select the first non-nil summary deterministically by sorted key, returning the
existing error when none exists. Delete the selected key and assign
"wrong-filter-key" only after a valid candidate is found, leaving the snapshot
unchanged on error.
In `@session/replaytest/harness_helper.go`:
- Around line 61-91: Sort the backend names deterministically before
constructing comparison pairs in both allPairs and referencePairs. When the
configured reference is absent, select the first name from the sorted list as
the fallback baseline, and iterate that same sorted list when appending pairs so
pair ordering and diff orientation remain stable.
In `@session/replaytest/sqlite/factory_extra_test.go`:
- Around line 18-31: Update the affected tests around TestOpen_TempDirAndCleanup
and the additional cleanup blocks to close the memory service before the session
service, rather than relying only on cleanup. Capture and assert errors from
both mem.Close and sess.Close where practical, while preserving the existing
cleanup idempotency coverage.
---
Nitpick comments:
In `@session/replaytest/backend_test.go`:
- Around line 22-25: Remove the raw os.Unsetenv call from the keys loop, leaving
t.Setenv(k, "") as the sole environment update so the test framework can track
and restore each variable.
In `@session/replaytest/profile_test.go`:
- Around line 10-21: Update TestProfiles_NamesAndCaps to define the expected
names for InMemoryProfile, SQLiteProfile, and RedisProfile, assert each
profile’s actual Name matches its expected value, and verify all profile names
are unique. Preserve the existing SupportsSessionState assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43572189-c51c-4c4b-afaf-39311d17f399
📒 Files selected for processing (14)
session/replaytest/backend_test.gosession/replaytest/comparator.gosession/replaytest/comparator_helper.gosession/replaytest/comparator_test.gosession/replaytest/coverage_comparator_test.gosession/replaytest/coverage_extra_test.gosession/replaytest/faultinject.gosession/replaytest/faultinject_helper.gosession/replaytest/harness.gosession/replaytest/harness_helper.gosession/replaytest/profile_test.gosession/replaytest/sqlite/factory_extra_test.gosession/replaytest/sqlite/go.modsession/replaytest/steps_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- session/replaytest/sqlite/go.mod
| if !okA || !okB || ta == nil || tb == nil { | ||
| add(fmt.Sprintf("tracks[%q]", name), okA, okB, "track presence mismatch") | ||
| return diffs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat equal nil track entries as equal.
When both maps contain name with nil values, this branch emits a mismatch with Baseline: true and Actual: true. It also misreports one-nil mismatches using identical values. Separate key presence from value presence.
中文
两侧均为 nil 的 Track 条目应判定为相等。
当两侧 map 都包含 name 且值均为 nil 时，此分支仍会生成差异，并显示 Baseline: true、Actual: true。单侧 nil 时也会产生误导值。请分别处理 key 与 value 的存在性。
Proposed fix
- if !okA || !okB || ta == nil || tb == nil {
+ if okA != okB {
add(fmt.Sprintf("tracks[%q]", name), okA, okB, "track presence mismatch")
return diffs
}
+ if !okA {
+ return diffs
+ }
+ if (ta == nil) != (tb == nil) {
+ add(fmt.Sprintf("tracks[%q]", name), ta != nil, tb != nil, "track value presence mismatch")
+ return diffs
+ }
+ if ta == nil {
+ return diffs
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !okA || !okB || ta == nil || tb == nil { | |
| add(fmt.Sprintf("tracks[%q]", name), okA, okB, "track presence mismatch") | |
| return diffs | |
| if okA != okB { | |
| add(fmt.Sprintf("tracks[%q]", name), okA, okB, "track presence mismatch") | |
| return diffs | |
| } | |
| if !okA { | |
| return diffs | |
| } | |
| if (ta == nil) != (tb == nil) { | |
| add(fmt.Sprintf("tracks[%q]", name), ta != nil, tb != nil, "track value presence mismatch") | |
| return diffs | |
| } | |
| if ta == nil { | |
| return diffs | |
| } |
| if !okA || !okB || ta == nil || tb == nil { | |
| add(fmt.Sprintf("tracks[%q]", name), okA, okB, "track presence mismatch") | |
| return diffs | |
| if okA != okB { | |
| add(fmt.Sprintf("tracks[%q]", name), okA, okB, "track presence mismatch") | |
| return diffs | |
| } | |
| if !okA { | |
| return diffs | |
| } | |
| if (ta == nil) != (tb == nil) { | |
| add(fmt.Sprintf("tracks[%q]", name), ta != nil, tb != nil, "track value presence mismatch") | |
| return diffs | |
| } | |
| if ta == nil { | |
| return diffs | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/comparator_helper.go` around lines 198 - 200, Update the
track comparison branch around the okA/okB and ta/tb checks to distinguish map
key presence from non-nil track values. Treat both keys present with nil values
as equal, report key-presence mismatches using okA and okB, and report one-nil
value mismatches with accurate value-presence booleans before returning diffs.
| if len(ta.Events) != len(tb.Events) { | ||
| add(fmt.Sprintf("tracks[%q].events.length", name), len(ta.Events), len(tb.Events), "track event count mismatch") | ||
| } | ||
| n := min(len(ta.Events), len(tb.Events)) | ||
| for i := 0; i < n; i++ { | ||
| if !bytes.Equal(ta.Events[i].Payload, tb.Events[i].Payload) { | ||
| add(fmt.Sprintf("tracks[%q].events[%d].payload", name, i), string(ta.Events[i].Payload), string(tb.Events[i].Payload), "track payload mismatch") | ||
| } | ||
| if !ta.Events[i].Timestamp.Equal(tb.Events[i].Timestamp) { | ||
| add(fmt.Sprintf("tracks[%q].events[%d].timestamp", name, i), ta.Events[i].Timestamp, tb.Events[i].Timestamp, "track timestamp mismatch") | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Compare track association fields as well as payload and time.
TrackEvents.Track and each TrackEvent.Track are observable association metadata. A backend can persist an event under the correct map key but corrupt these fields without producing a diff.
中文
除 payload 和时间外，还需比较 Track 关联字段。
TrackEvents.Track 及每个 TrackEvent.Track 都是可观察的关联元数据。后端可能使用正确 map key，却错误持久化这些字段，而当前不会产生差异。
As per path instructions, Track replay must verify events and their associated information.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/comparator_helper.go` around lines 202 - 213, Extend the
track comparison logic around the event count and payload/timestamp checks to
also compare TrackEvents.Track and each TrackEvent.Track. Record mismatches
through add using the existing tracks event path conventions, while preserving
the current payload, timestamp, and length comparisons.
Source: Path instructions
| var diffs []Diff | ||
| backendA := snapshotBackend(a, profileA) | ||
| backendB := snapshotBackend(b, profileB) | ||
| sessionID := compareSessionID(a, b) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Compare session identity instead of only selecting it as report metadata.
Line 44 chooses one ID for annotations, but never reports differing IDs. A backend may drop or rewrite the session ID while replay still passes. Compare the effective IDs from both snapshots and emit a session.id diff.
中文
应比较 Session 标识，而不仅将其用作报告元数据。
第 44 行仅选择一个 ID 用于标注，却未报告两侧 ID 不同。后端即使丢失或改写 Session ID，replay 仍会通过。请比较两侧有效 ID，并生成 session.id 差异。
This conflicts with the PR objective to verify Session replay consistency and precisely locate differences.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/comparator.go` at line 44, Update the comparison flow
around compareSessionID to compare the effective session IDs from both snapshots
and emit a session.id diff when they differ, rather than using the selected ID
only as report metadata. Preserve the existing ID selection for annotations
while ensuring missing, dropped, or rewritten IDs are reported.
| // ensure original not fully shared for state map mutation safety loosely | ||
| out.AppState["a"] = []byte("mut") | ||
| if string(snap.AppState["a"]) == "mut" { | ||
| // clone should isolate; if not, still don't fail hard — just note | ||
| t.Log("app state may be shared") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Fail when normalization shares mutable state.
Clone isolation is part of the normalizer contract; logging allows a regression that can mutate raw snapshots and contaminate later comparisons.
中文
Normalizer 共享可变状态时应使测试失败。
克隆隔离属于 Normalizer 的核心契约。这里只记录日志会放过回归，导致原始 snapshot 被修改并污染后续比较。
Proposed fix
out.AppState["a"] = []byte("mut")
if string(snap.AppState["a"]) == "mut" {
- t.Log("app state may be shared")
+ t.Fatal("normalized app state shares storage with the source snapshot")
}As per path instructions, tests must use assertions strong enough to verify intended behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ensure original not fully shared for state map mutation safety loosely | |
| out.AppState["a"] = []byte("mut") | |
| if string(snap.AppState["a"]) == "mut" { | |
| // clone should isolate; if not, still don't fail hard — just note | |
| t.Log("app state may be shared") | |
| out.AppState["a"] = []byte("mut") | |
| if string(snap.AppState["a"]) == "mut" { | |
| t.Fatal("normalized app state shares storage with the source snapshot") |
| // ensure original not fully shared for state map mutation safety loosely | |
| out.AppState["a"] = []byte("mut") | |
| if string(snap.AppState["a"]) == "mut" { | |
| // clone should isolate; if not, still don't fail hard — just note | |
| t.Log("app state may be shared") | |
| out.AppState["a"] = []byte("mut") | |
| if string(snap.AppState["a"]) == "mut" { | |
| t.Fatal("normalized app state shares storage with the source snapshot") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/coverage_extra_test.go` around lines 532 - 536, Update the
clone-isolation assertion in the normalization test around out.AppState and
snap.AppState so shared mutable state causes the test to fail rather than merely
logging with t.Log. Assert that mutating out.AppState does not change
snap.AppState, preserving the normalizer’s isolation contract.
Source: Path instructions
| var sum *session.Summary | ||
| if v, ok := snap.Session.Summaries[""]; ok { | ||
| sum = v | ||
| delete(snap.Session.Summaries, "") | ||
| } else { | ||
| keys := make([]string, 0, len(snap.Session.Summaries)) | ||
| for k := range snap.Session.Summaries { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
| if len(keys) == 0 { | ||
| return fmt.Errorf("no summary to rekey") | ||
| } | ||
| k := keys[0] | ||
| sum = snap.Session.Summaries[k] | ||
| delete(snap.Session.Summaries, k) | ||
| } | ||
| if sum == nil { | ||
| return fmt.Errorf("no summary to rekey") | ||
| } | ||
| snap.Session.Summaries["wrong-filter-key"] = sum |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Select a non-nil summary before mutating the map.
If the empty-key entry is nil, it is deleted before the function returns an error—even when another valid summary exists. Select a deterministic non-nil candidate first, then delete and rekey it so error paths leave the snapshot unchanged.
中文
修改 map 前应先选择非 nil 的 Summary。
如果空 filter-key 对应 nil，该条目会先被删除，随后函数才返回错误；即使 map 中还有其他有效 Summary 也会失败。请先确定性地选择非 nil 候选，再执行删除和换 key，确保错误路径不修改 snapshot。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/faultinject_helper.go` around lines 75 - 95, Update the
summary-selection logic around the empty-key and sorted-key branches to choose a
non-nil *session.Summary before mutating snap.Session.Summaries. Prefer the
empty-key entry only when non-nil; otherwise select the first non-nil summary
deterministically by sorted key, returning the existing error when none exists.
Delete the selected key and assign "wrong-filter-key" only after a valid
candidate is found, leaving the snapshot unchanged on error.
| func allPairs(snaps map[string]*Snapshot) [][2]string { | ||
| names := make([]string, 0, len(snaps)) | ||
| for n := range snaps { | ||
| names = append(names, n) | ||
| } | ||
| var pairs [][2]string | ||
| for i := 0; i < len(names); i++ { | ||
| for j := i + 1; j < len(names); j++ { | ||
| pairs = append(pairs, [2]string{names[i], names[j]}) | ||
| } | ||
| } | ||
| return pairs | ||
| } | ||
|
|
||
| func referencePairs(reference string, snaps map[string]*Snapshot) [][2]string { | ||
| ref := reference | ||
| if _, ok := snaps[ref]; !ok { | ||
| // pick first as reference | ||
| for n := range snaps { | ||
| ref = n | ||
| break | ||
| } | ||
| } | ||
| var pairs [][2]string | ||
| for n := range snaps { | ||
| if n == ref { | ||
| continue | ||
| } | ||
| pairs = append(pairs, [2]string{ref, n}) | ||
| } | ||
| return pairs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sort backend names before selecting comparison pairs.
Map iteration makes allPairs ordering unstable. More critically, when the configured reference is absent, referencePairs chooses a random baseline, changing diff orientation between runs. This affects custom backend names and deterministic JSON reports.
中文
选择比较配对前应先排序后端名称。
直接遍历 map 会导致 allPairs 顺序不稳定。更严重的是，当配置的 reference 不存在时，referencePairs 会随机选择 baseline，导致不同运行中的差异方向发生变化。这会影响自定义后端名称和确定性 JSON 报告。
Proposed fix
import (
"context"
"fmt"
+ "sort"
)
+func sortedSnapshotNames(snaps map[string]*Snapshot) []string {
+ names := make([]string, 0, len(snaps))
+ for name := range snaps {
+ names = append(names, name)
+ }
+ sort.Strings(names)
+ return names
+}
+
func allPairs(snaps map[string]*Snapshot) [][2]string {
- names := make([]string, 0, len(snaps))
- for n := range snaps {
- names = append(names, n)
- }
+ names := sortedSnapshotNames(snaps)
func referencePairs(reference string, snaps map[string]*Snapshot) [][2]string {
+ names := sortedSnapshotNames(snaps)
ref := reference
if _, ok := snaps[ref]; !ok {
- for n := range snaps {
- ref = n
- break
- }
+ ref = names[0]
}
- for n := range snaps {
+ for _, n := range names {As per path instructions, framework defaults and externally observable results must remain stable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func allPairs(snaps map[string]*Snapshot) [][2]string { | |
| names := make([]string, 0, len(snaps)) | |
| for n := range snaps { | |
| names = append(names, n) | |
| } | |
| var pairs [][2]string | |
| for i := 0; i < len(names); i++ { | |
| for j := i + 1; j < len(names); j++ { | |
| pairs = append(pairs, [2]string{names[i], names[j]}) | |
| } | |
| } | |
| return pairs | |
| } | |
| func referencePairs(reference string, snaps map[string]*Snapshot) [][2]string { | |
| ref := reference | |
| if _, ok := snaps[ref]; !ok { | |
| // pick first as reference | |
| for n := range snaps { | |
| ref = n | |
| break | |
| } | |
| } | |
| var pairs [][2]string | |
| for n := range snaps { | |
| if n == ref { | |
| continue | |
| } | |
| pairs = append(pairs, [2]string{ref, n}) | |
| } | |
| return pairs | |
| func sortedSnapshotNames(snaps map[string]*Snapshot) []string { | |
| names := make([]string, 0, len(snaps)) | |
| for name := range snaps { | |
| names = append(names, name) | |
| } | |
| sort.Strings(names) | |
| return names | |
| } | |
| func allPairs(snaps map[string]*Snapshot) [][2]string { | |
| names := sortedSnapshotNames(snaps) | |
| var pairs [][2]string | |
| for i := 0; i < len(names); i++ { | |
| for j := i + 1; j < len(names); j++ { | |
| pairs = append(pairs, [2]string{names[i], names[j]}) | |
| } | |
| } | |
| return pairs | |
| } | |
| func referencePairs(reference string, snaps map[string]*Snapshot) [][2]string { | |
| names := sortedSnapshotNames(snaps) | |
| ref := reference | |
| if _, ok := snaps[ref]; !ok { | |
| // pick first as reference | |
| ref = names[0] | |
| } | |
| var pairs [][2]string | |
| for _, n := range names { | |
| if n == ref { | |
| continue | |
| } | |
| pairs = append(pairs, [2]string{ref, n}) | |
| } | |
| return pairs | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/harness_helper.go` around lines 61 - 91, Sort the backend
names deterministically before constructing comparison pairs in both allPairs
and referencePairs. When the configured reference is absent, select the first
name from the sorted list as the fallback baseline, and iterate that same sorted
list when appending pairs so pair ordering and diff orientation remain stable.
Source: Path instructions
| func TestOpen_TempDirAndCleanup(t *testing.T) { | ||
| sess, mem, profile, cleanup, err := replaysqlite.Open("") | ||
| if err != nil { | ||
| t.Skipf("sqlite backend unavailable: %v", err) | ||
| } | ||
| if profile.Name != "sqlite" { | ||
| t.Fatalf("profile=%s", profile.Name) | ||
| } | ||
| if sess == nil || mem == nil { | ||
| t.Fatal("nil services") | ||
| } | ||
| // cleanup should be idempotent | ||
| cleanup() | ||
| cleanup() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close SQLite services before removing their directory.
The first two tests invoke only cleanup, leaving database handles open. The factory cleanup closes sess first, but its Close triggers directory removal while mem may still hold memory.db. Close memory first, then session, and assert close errors where practical.
中文
删除 SQLite 目录前应先关闭所有服务。
前两个测试仅调用 cleanup，数据库句柄仍保持打开。Factory 测试又先关闭 sess，但其 Close 会在 mem 仍可能持有 memory.db 时清理目录。请先关闭 memory，再关闭 session，并尽量断言关闭错误。
Also applies to: 34-40, 61-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@session/replaytest/sqlite/factory_extra_test.go` around lines 18 - 31, Update
the affected tests around TestOpen_TempDirAndCleanup and the additional cleanup
blocks to close the memory service before the session service, rather than
relying only on cleanup. Capture and assert errors from both mem.Close and
sess.Close where practical, while preserving the existing cleanup idempotency
coverage.
Rememorio
left a comment
Rememorio
left a comment
There was a problem hiding this comment.
I reviewed the changed lines and left 7 inline comments below. The comments focus on issues worth addressing before merge.
Fresh review found seven high-confidence issues: shared-state races in parallel execution, swallowed session-read errors, false passes with no backends, missing session and memory ownership comparisons, nondeterministic reference fallback, and SQLite initialization failures being skipped. Five previous concerns are fixed; the recovery scenario is partially fixed because its test still does not assert event multiplicity. The most useful validations are race-enabled parallel tests, error-injecting session fakes, identity-mismatch comparator tests, and a non-skipping SQLite integration test.
中文
我看过这次变更的相关代码，在下面留下 7 条行内评论。评论聚焦在合并前值得处理的问题。
本次复审发现 7 个高置信度问题：并行执行会竞态修改共享状态、会话读取错误被吞掉、未注册后端时仍会假通过、会话与内存归属字段未参与比较、基准后端回退不确定，以及 SQLite 初始化故障会被直接跳过。历史评论中 5 条已修复；恢复场景仅部分修复，因为测试仍未断言事件数量。建议重点补充 race 模式下的并行测试、注入 GetSession 错误的 fake、身份字段差异测试，以及不会吞掉初始化错误的 SQLite 集成测试。
| }) | ||
| continue | ||
| } | ||
| ca, cb := memoryContent(a[i]), memoryContent(b[i]) |
There was a problem hiding this comment.
P1: Compare memory ownership fields
Memory comparison starts with payload content and never checks memory.Entry.AppName or UserID, although the normalizer preserves both fields. A backend that reads the correct content from the wrong tenant, or labels a returned row with the wrong owner, will compare equal.
Include both ownership fields in the semantic comparison and add a fixture where only AppName or UserID differs. This is necessary for the replay harness to catch cross-tenant persistence regressions.
中文
内存记录的归属字段也需要比较
内存比较从内容字段开始，却没有检查 memory.Entry.AppName 和 UserID，而规范化过程实际上保留了这两个字段。这样一来，后端若从错误租户读取了相同内容，或给返回记录写入了错误归属，仍会被判定一致。
建议把这两个归属字段纳入语义比较，并增加仅修改 AppName 或 UserID 的 fixture。否则该框架无法发现跨租户持久化错误。
| "trpc.group/trpc-go/trpc-agent-go/session" | ||
| ) | ||
|
|
||
| func compareSessionID(a, b *Snapshot) string { |
There was a problem hiding this comment.
P1: Compare session identity instead of using it only as a locator
compareSessionID only selects an ID for report metadata; the comparator never checks Snapshot.SessionID or Session.ID, AppName, and UserID. A backend that returns another session or user's record with otherwise identical events and state will therefore pass consistency validation.
Compare the semantic identity fields explicitly, normalizing only values that are genuinely backend-generated. Add a regression that changes each identity field while keeping the payload unchanged and requires a non-allowed diff.
中文
会话身份不能只作为报告定位信息
compareSessionID 只是选择一个 ID 填入报告，比较器并没有检查 Snapshot.SessionID，也没有比较 Session.ID、AppName 和 UserID。因此，后端即使返回了其他 session 或其他用户的记录，只要事件和状态内容相同，也会通过一致性校验。
建议显式比较这些身份字段，只对确实由后端生成的值做规范化。应增加回归测试：保持业务内容不变，分别修改各身份字段，并断言产生不可忽略的 diff。
| return cr, nil | ||
| } | ||
|
|
||
| func validateBackends(backends []NamedBackend) error { |
There was a problem hiding this comment.
P1: Reject runs with no registered backend
validateBackends accepts an empty backend list. Each CaseResult starts as passed, and the zero-snapshot branch returns without changing that status, so Run reports every supplied case as passed even though no step or comparison ran.
Reject an empty backend list as a configuration error and add a regression asserting that an unconfigured harness cannot produce a green report.
中文
空后端配置会产生假通过
validateBackends 会接受空的后端列表。由于每个 CaseResult 初始状态就是 passed，零快照分支返回时又不会修改状态，最终所有传入用例都会被报告为通过，实际上既没有执行步骤，也没有发生任何比较。
建议把空后端列表视为配置错误，并增加回归测试，确保未配置后端的 harness 无法生成绿色报告。
| } | ||
| e.mu.Unlock() | ||
| // Prefer backend as source of truth outside the lock (may block). | ||
| if existing, err := e.backend.SessionService.GetSession(ctx, key); err == nil && existing != nil { |
There was a problem hiding this comment.
P1: Propagate GetSession failures before creating
ensureSession treats every GetSession error as an absent session and falls through to CreateSession. A database outage, decoding failure, or cancellation can therefore be masked by a create attempt, which may also mutate storage despite the failed read. createSummary and appendTrack repeat the same error-discarding pattern when refreshing the session.
The existing services represent not-found as (nil, nil), so non-nil errors should be returned unchanged. Add a fake service whose GetSession fails and assert that CreateSession is not invoked and the original error remains distinguishable.
中文
不要把 GetSession 错误当作未命中
ensureSession 会把所有 GetSession 错误都当成“session 不存在”，随后继续调用 CreateSession。数据库故障、解码失败或取消错误因此可能被创建操作掩盖，甚至在读取失败后仍修改存储。createSummary 和 appendTrack 刷新 session 时也有相同的吞错逻辑。
现有后端使用 (nil, nil) 表示未找到，因此非空错误应直接向上传递。建议增加一个让 GetSession 失败的 fake，断言不会调用 CreateSession，并且调用方仍能识别原始错误。
| for _, branch := range step.Branches { | ||
| br := branch | ||
| workers.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
P1: Parallel branches race on shared executor state
ParallelGroupStep calls e.execute concurrently on the same caseExecutor, but executor-owned state is not consistently synchronized. The built-in concurrent case has both branches assign e.snapshot.SessionID; other supported nested steps also write snapshot and sessions without the mutex. This is a real Go data race. A custom group whose branches initialize the same new session can additionally issue duplicate CreateSession calls because initialization is not atomic.
Keep backend operations concurrent while protecting executor state—for example, use branch-local results with a deterministic merge and single initialization per session key. Add a race-enabled regression for CaseConcurrentInterleaved and for parallel first writes to a new session.
中文
并行分支正在竞态修改共享执行器状态
ParallelGroupStep 会让多个 goroutine 在同一个 caseExecutor 上调用 e.execute，但执行器自身的状态并没有完整同步。内置并发用例的两个分支都会写 e.snapshot.SessionID；其他允许嵌套的步骤也会在未加锁的情况下修改 snapshot 或 sessions，因此这里存在真实的 Go 数据竞态。若两个分支同时首次访问同一个新 session，当前初始化窗口还可能重复调用 CreateSession。
建议保持后端操作并发，但隔离或同步执行器状态，例如让各分支使用独立结果并确定性合并，同时确保每个 session key 只初始化一次。应增加 race 模式下的 CaseConcurrentInterleaved 回归测试，以及两个分支同时首次写入新 session 的场景。
|
|
||
| func referencePairs(reference string, snaps map[string]*Snapshot) [][2]string { | ||
| ref := reference | ||
| if _, ok := snaps[ref]; !ok { |
There was a problem hiding this comment.
P2: Do not silently replace a missing reference backend
When the configured reference has no snapshot, referencePairs silently picks the first map entry. With default options and no inmemory backend, the effective baseline is therefore nondeterministic, while Report.Reference still says inmemory. The same problem occurs when the configured reference is capability-skipped for a case.
Reject a missing reference, or explicitly propagate the effective reference into the report and make selection deterministic. Add coverage for an absent and a capability-skipped reference backend.
中文
不要静默替换缺失的基准后端
当配置的基准后端没有快照时，referencePairs 会静默选择 map 中的第一个条目。使用默认配置但未注册 inmemory 时，实际基准将取决于不稳定的 map 遍历顺序，而 Report.Reference 仍然显示 inmemory。基准后端因能力不足而跳过某个用例时也会出现同样问题。
建议在基准缺失时直接报错，或者显式记录并上报实际采用的基准，同时保证选择顺序确定。应覆盖“基准未注册”和“基准因能力不足被跳过”两种场景。
| sess, mem, profile, cleanup, err := replaysqlite.Open(t.TempDir()) | ||
| if err != nil { | ||
| // CGO/sqlite driver may be unavailable on some hosts. | ||
| t.Skipf("sqlite backend unavailable: %v", err) |
There was a problem hiding this comment.
P2: Fail the SQLite integration test on initialization regressions
The only InMemory-versus-SQLite replay test skips on every error returned by Open. A schema, migration, path, or service-construction regression will therefore appear as an environmental skip instead of failing the dual-backend validation.
Gate this test with an appropriate CGO build constraint, or skip only the known unsupported-driver error and fail all other initialization errors. This keeps environments without CGO supported without hiding functional SQLite breakage.
中文
SQLite 初始化回归不应被标记为跳过
唯一的 InMemory 与 SQLite 对照测试会对 Open 返回的任何错误执行 Skip。因此，schema、迁移、路径或服务构造发生回归时，测试结果会显示为环境不支持，而不是让双后端校验失败。
建议使用合适的 CGO 构建约束，或者只对明确的“不支持 CGO 驱动”错误执行跳过，其余初始化错误都应失败。这样既能兼容无 CGO 环境，也不会掩盖 SQLite 的功能性故障。
3 participants
问题 / 根因分析
issue #2001 要求给 Session / Memory / Summary / Track 做跨后端 replay 一致性校验。现状里几个后端都有各自单测，但缺统一入口：
方案
新增纯测试侧包
session/replaytest，生产代码零改动。1. 流水线
默认用 InMemory 作 reference；目标后端快照与 reference 规范化后比较。
2. Step 抽象
用
ReplayStep描述业务动作，而不是手写后端调用。覆盖：3. Normalizer
统一处理：
避免“逻辑一致但字段格式不同”的假失败。
4. Comparator + AllowedDiff
字段级 diff，路径可定位到：
session_id/event_indexsummary_filter_key/track_name/memory_idpathAllowedDiff 策略：
ignorewithin_deltanot_emptysame_type5. Report
JSON 报告字段包括：case 名、backend、status、failed 数、diffs 明细、耗时等；
testdata/sample_report.json给了样例结构，方便 CI 或本地对照。6. 11 个公开 case
single_turn_textmulti_turn_conversationtool_call_conversationstate_crudmemory_write_and_readsummary_generationsummary_with_truncationsummary_filter_keytrack_eventsconcurrent_interleavedrecovery_duplicate_event7. Fault injection
内置 11 类故障注入，正常路径应 0 failed，注入后应 100% 检出，包括：
summary 四类错误拆开断言，不混成一个“summary 不对”。
8. 后端接入
session/replaytest/sqlite独立 module：提供 SQLite factory，与上游session/sqlite、memory/sqlite子模块模式一致9. Summarizer
使用确定性
FakeSummarizer，不调真实 LLM，保证 summary case 可重复、可 CI。主要变更
仅新增
session/replaytest/**，不修改既有生产实现：28 files, +3483 行。
测试
本地已跑：
./session/replaytest/session/replaytest/sqlite覆盖：
验收对照
faultinject+ 测试session/replaytest/**Closes #2001