session/replaytest: add multi-backend replay consistency framework#2265
session/replaytest: add multi-backend replay consistency framework#2265Wsp030914 wants to merge 6 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a reusable Go replay-consistency framework with typed snapshots, ten standard scenarios, normalization, semantic comparison, deterministic reports, backend fixtures, capability probes, integration matrices, and write-boundary fault tests across Session and Memory implementations. ChangesReplay consistency framework
Estimated code review effort: 5 (Critical) | ~120 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2265 +/- ##
===================================================
+ Coverage 89.86515% 89.87964% +0.01448%
===================================================
Files 1148 1157 +9
Lines 202302 205299 +2997
===================================================
+ Hits 181799 184522 +2723
- Misses 12815 13009 +194
- Partials 7688 7768 +80
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
test/go.mod (1)
8-100: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winBump
go.opentelemetry.io/otel/sdkto v1.43.0+
test/go.modstill pullsgo.opentelemetry.io/otel/sdk v1.41.0, which falls in GHSA-hfvc-g4fc-pqhx / CVE-2026-39883. The fix is small and removes the BSD/SolariskenvPATH-hijack risk.中文
将
go.opentelemetry.io/otel/sdk升级到 v1.43.0+
test/go.mod里仍然引用了go.opentelemetry.io/otel/sdk v1.41.0，落在 GHSA-hfvc-g4fc-pqhx / CVE-2026-39883 的影响范围内。这个修复很小，可以移除 BSD/Solaris 下kenv的 PATH 劫持风险。🤖 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 `@test/go.mod` around lines 8 - 100, Update the go.opentelemetry.io/otel/sdk dependency in the test module’s indirect require block from v1.41.0 to v1.43.0 or newer, keeping the dependency declaration otherwise unchanged.
🧹 Nitpick comments (2)
session/replaytest/runner.go (1)
23-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the
Fixtureconcurrency contractexecuteParallelinvokesfixture.Apply/ApplyWithFaultfrom multiple goroutines, andFixtureis a public interface. Add a doc note that implementations must be safe for concurrent use so downstream backends don’t assume single-threaded access and race on shared state.中文
为
Fixture补充并发契约说明。executeParallel会从多个 goroutine 并发调用fixture.Apply/ApplyWithFault，而Fixture是公开接口。建议在注释中明确实现必须支持并发调用，避免下游后端误以为是单线程访问而在共享状态上产生竞态。🤖 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/runner.go` around lines 23 - 35, Update the documentation comment for the public Fixture interface to state that implementations must be safe for concurrent use because executeParallel may invoke Apply and ApplyWithFault from multiple goroutines. Leave the interface methods and FaultInjector documentation unchanged.test/replayconsistency/write_fault_test.go (1)
383-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueManually-maintained clone whitelist could silently drift from
Operation's reference fields.
cloneWriteOperationdeep-clones a fixed list of reference-typed fields. IfOperation,EventSnapshot,MemorySnapshot,SummarySnapshot, orTrackEventSnapshotgain new map/slice fields later, this helper won't clone them automatically, silently reintroducing the cross-backend aliasing this helper exists to prevent. Worth a short comment here noting the maintenance obligation, or verifying test coverage catches drift when new fields are added.中文
手动维护的克隆白名单可能与
Operation的引用类型字段产生偏差。
cloneWriteOperation对固定的一组引用类型字段做深拷贝。若未来Operation、EventSnapshot、MemorySnapshot、SummarySnapshot或TrackEventSnapshot新增 map/slice 字段，该辅助函数不会自动克隆它们，会悄然重新引入该函数本应防止的跨后端别名污染问题。建议在此处补充维护提醒注释，或确认新增字段时有测试能捕获遗漏。🤖 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 `@test/replayconsistency/write_fault_test.go` around lines 383 - 420, Add a concise maintenance comment to cloneWriteOperation documenting that its explicit deep-clone list must be updated whenever Operation or its nested snapshot types gain map or slice fields, preserving the helper’s alias-isolation contract.
🤖 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 `@test/go.mod`:
- Around line 8-100: Update the go.opentelemetry.io/otel/sdk dependency in the
test module’s indirect require block from v1.41.0 to v1.43.0 or newer, keeping
the dependency declaration otherwise unchanged.
---
Nitpick comments:
In `@session/replaytest/runner.go`:
- Around line 23-35: Update the documentation comment for the public Fixture
interface to state that implementations must be safe for concurrent use because
executeParallel may invoke Apply and ApplyWithFault from multiple goroutines.
Leave the interface methods and FaultInjector documentation unchanged.
In `@test/replayconsistency/write_fault_test.go`:
- Around line 383-420: Add a concise maintenance comment to cloneWriteOperation
documenting that its explicit deep-clone list must be updated whenever Operation
or its nested snapshot types gain map or slice fields, preserving the helper’s
alias-isolation contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b4cfcc8-5e33-45b7-a859-a4232fb9d8cc
⛔ Files ignored due to path filters (1)
test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
session/replaytest/README.mdsession/replaytest/cases.gosession/replaytest/cases_test.gosession/replaytest/compare.gosession/replaytest/compare_test.gosession/replaytest/detection_test.gosession/replaytest/doc.gosession/replaytest/normalize.gosession/replaytest/normalize_test.gosession/replaytest/operation.gosession/replaytest/operation_test.gosession/replaytest/report.gosession/replaytest/report_test.gosession/replaytest/runner.gosession/replaytest/runner_test.gosession/replaytest/types.gosession/replaytest/types_test.gotest/go.modtest/replayconsistency/README.mdtest/replayconsistency/capability_probe.gotest/replayconsistency/capability_probe_test.gotest/replayconsistency/external_matrix_test.gotest/replayconsistency/factory.gotest/replayconsistency/factory_test.gotest/replayconsistency/fixture.gotest/replayconsistency/fixture_adapter_test.gotest/replayconsistency/integration_test.gotest/replayconsistency/replay_test.gotest/replayconsistency/state_semantics_test.gotest/replayconsistency/testdata/session_memory_summary_track_diff_report.jsontest/replayconsistency/write_fault_test.go
|
All contributors have signed the CLA ✍️ ✅ |
46c78c2 to
825be4c
Compare
|
Addressed all review feedback in 825be4c.
|
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. The main framework can silently produce false passes because nested operation payloads are shared across backends, unused allowed-diff rules are accepted, physical session ownership is overwritten without validation, and memory searches execute against final rather than point-in-time state. The integration probes also need stronger seed assertions and bounded pagination contexts.
中文
我看过这次变更的相关代码，在下面留下 7 条行内评论。评论聚焦在合并前值得处理的问题。
本次审阅发现 7 个高置信度问题。框架可能因操作负载在后端间共享、未命中的 allowed-diff 规则仍被接受、物理 Session 归属未经校验就被覆盖，以及 Memory Search 延迟到最终快照才执行而出现静默误判。集成探针还需要更严格的种子数据断言和有界的分页测试上下文。
Thanks for the thorough review. All seven issues have been addressed in commit 4446d78:
Local unit, race, vet, lint, and Redis/PostgreSQL/MySQL/ClickHouse integration checks passed. No workflow or unrelated dependency files were changed. |
3 participants
What changed
session/replaytest.tracks, concurrent writes, and failure recovery.
and ClickHouse integrations enabled through environment variables.
snapshot invariants, and structured JSON reports.
integration tests, and running documentation.
Why
Session and Memory backends can preserve different replay-visible behavior
for the same agent trajectory. This framework detects event ordering, state,
memory, summary, and track inconsistencies before backend migration or release.
Fixes #2001
Testing
go test ./session/replaytest -count=1— 100 tests passed.go test -race ./session/replaytest -count=1— 100 tests passed.cd test && go test ./replayconsistency -count=1— 54 tests passed.cd test && go test -race ./replayconsistency -count=1— 54 tests passed.go vetpassed for both packages.Notes for reviewers
comparison, concurrent replay ordering, retry idempotency, and allowed-diff
precision.
configured.
test/go.moddependencies come from the Redis, Postgres,MySQL, ClickHouse, and SQLite adapters.
RELEASE NOTES: Added a reusable multi-backend replay consistency test framework
for Session, Memory, Summary, and Track backends.