feat(replay): compare the behaviour of two recorded sessions - #3948
feat(replay): compare the behaviour of two recorded sessions#3948dwin-gharibi wants to merge 2 commits into
Conversation
aheritier
left a comment
There was a problem hiding this comment.
Summary
Real gap, correct core insight, well-built package — but it reports a false "identical" verdict on multi-agent sessions, and adding a new top-level CLI verb needs a maintainer decision first.
The two design calls the description leads with are the hard part, and they're right: comparing tool calls rather than prose, and stopping at the first divergence. pkg/replay is pure, genuinely well-documented, and 92.2% covered by tests that encode those decisions (ProseIsNeverADivergence, order-matters, first-divergence-only) rather than just exercising lines. The scope section is honest about what was left out.
Findings below are all reproduced, not read off.
[blocking] Delegated (sub-session) turns are dropped, producing a false "Identical behaviour"
pkg/replay/replay.go:100-103 skips every item where item.Message == nil. But delegated work lives in Item.SubSession *Session — "a complete sub-session from task transfers" (pkg/session/session.go:102-103). In any multi-agent run, the sub-agent's entire tool-call sequence is invisible to the comparison.
Failing case:
func TestFinding_SubSessionTurnsAreIgnored(t *testing.T) {
mk := func(subTool string) *session.Session {
return &session.Session{Messages: []session.Item{
asst("transfer_task", `{"agent":"coder"}`),
{SubSession: &session.Session{Messages: []session.Item{
asst(subTool, `{"path":"main.go"}`),
}}},
}}
}
a, b := mk("read_file"), mk("shell") // sub-agents did entirely different things
require.Len(t, replay.TurnsOf(a), 1) // passes: sub-session turn dropped
assert.False(t, replay.CompareSessions(a, b).Identical()) // FAILS
}--- FAIL: TestFinding_SubSessionTurnsAreIgnored
Error: Should be false
Messages: runs whose sub-agents diverged must not be reported as identical behaviour
This is the one worth holding the PR on. The command prints ✅ Identical behaviour across all N turns. for two runs that behaved differently, and the user's takeaway is "the agent isn't what changed" — the precise wrong conclusion for the triage question in #3947.
The traversal pattern already exists in-tree: pkg/tui/dialog/cost.go:305-308 and pkg/tui/service/sessionstate.go:308-309 both recurse via item.IsSubSession() / item.SubSession. Either flatten sub-session assistant turns in order, or detect them and refuse rather than silently ignoring them. Needs a test either way — no current test constructs a SubSession item (the TurnsOf test covers user/tool/empty items only).
Related: the comment on Turn.Agent (replay.go:60-62) says it exists "so a divergence in a multi-agent run can be attributed", but multi-agent delegation is exactly the case the extraction cannot see.
[should-fix] Session references aren't resolved, so the primary workflow doesn't work
cmd/root/replay.go:74,78 calls store.GetSession(ctx, args[0]) directly, bypassing session.ResolveSessionID (pkg/session/store.go:57) — which every other session-consuming path uses: cmd/root/backend.go:159, cmd/root/run.go:947, pkg/runtime/sessioncontext_handlers.go:69.
So "compare my last two runs", the scenario in #3947:
$ docker agent replay -s ./session.db -- -1 -2
Error: reading session "-1": session not foundAnd the invocation advertised in both this description and #3947 fails too, since IDs are UUIDs and the lookup is exact WHERE id = ?:
$ docker agent replay -s ./session.db 3f8e173c fc9bf11c
Error: reading session "3f8e173c": session not foundFull UUIDs work correctly, as do --json and --fail-on-divergence. But there's no sessions ls command, so exact UUIDs are the hardest thing for a user to obtain. Routing both args through ResolveSessionID is about one line each and makes the feature usable day to day.
[should-fix] Model-controlled text reaches the terminal unsanitized
truncateArgs (pkg/replay/replay.go:218-226) strips \n and nothing else; printCalls writes the result straight out. \r and ESC survive:
"...\n old called:\n read_file({\"cmd\":\"safe\"}\r\x1b[2K\x1b[31m shell({\"cmd\":\"rm -rf /\"})\x1b[0m)\n..."
Tool arguments are model-generated and routinely carry content the agent read from untrusted files or the web, so the diagnostic's output can be shaped by the material being diagnosed — \r plus \x1b[2K erases the line and redraws a different tool call. The repo already does this hygiene: sanitizeForTerminal (pkg/tui/components/markdown/fast_renderer.go:2715) strips \r, \b, \f, \v for the same reason.
While there: truncation is per-call, so the "a large payload cannot flood the output" property (and its test) doesn't hold for a turn with many parallel tool calls — N × 120 runes is unbounded in N.
[should-fix] Byte-exact argument comparison reports false divergences
sameBehaviour (replay.go:163-174) compares ToolCall values, making Arguments a raw string compare. Semantically identical JSON diverges:
a := asst("read_file", `{"path":"a.go","limit":10}`)
b := asst("read_file", `{"limit":10,"path":"a.go"}`)
// replay.CompareSessions(a, b).Identical() == false--- FAIL: TestFinding_SemanticallyEqualArgsReportedAsDivergence
Error: Should be true
Messages: same tool, same arguments in a different key order is the same behaviour
This is the same category of nondeterminism noise that "compare behaviour, not prose" exists to eliminate, so leaving it byte-exact undercuts the PR's own thesis. Unmarshal to any and compare semantically, falling back to string equality when either side isn't valid JSON.
[should-fix] run is 0% covered and --fail-on-divergence has no test
cmd/root/replay.go:55: run 0.0%
cmd/root/replay.go:94: renderReplay 100.0%
Extracting the renderReplay seam was the right call, but the non-zero exit is the whole contract of --fail-on-divergence for the CI use case it exists for, and nothing pins it — same for the reading session %q wrapping. It works today (I get exit=1), which is exactly why it deserves a test before it can regress quietly. The KindExtraTurn / KindMissingTurn print branches are also unexercised (PrintResult 75%); both render correctly, they're just unpinned.
[should-fix] New top-level command isn't documented
docs/features/cli/index.md carries a ### docker agent <cmd> entry for every top-level command, including the closest analogue docker agent eval at line 450. This PR adds a command and a group entry (cmd/root/root.go:180) with no docs change.
[should-fix] The name collides with what "replay" already means here
pkg/recording/recording.go:1 — "recording and replaying AI API interactions" — and --record writes cassettes (cmd/root/api.go:48). This command doesn't replay anything; it diffs two recordings. #3947 also reserves the word for the future re-run-against-another-model feature, which would then need a different name than the thing it actually is. docker agent sessions diff (or compare) says what it does and keeps replay free for the replay.
[optional] Role compared against a string literal
replay.go:106: if msg.Role != "assistant". This is the only non-test occurrence in the tree; 45 non-test sites use chat.MessageRoleAssistant (pkg/chat/chat.go:25), and this PR's own tests use the constant.
On whether this should land at all
The gap is real: sessions are persisted in full, nothing compares two of them, and pkg/recording / cassettes / pkg/evaluation genuinely solve different problems. The scoping rationale is sound, and Compare really is the substrate a re-run feature would reuse. As a package this is low-risk — new code, no existing behaviour touched.
Two things I'd want settled before merge:
A maintainer needs to want the command. #3947 was opened by the same author, has no comments, and no maintainer has agreed to a new top-level verb. Whether this is docker agent replay, sessions diff, a subcommand under eval, or an affordance in the session picker is a product call about CLI surface, not something a PR should settle on its own. The package can land regardless; the verb shouldn't land silently.
The description promises more than the code delivers. #3947 frames the question as "which variable moved: the model's decisions, or the environment it ran in" — and this cannot answer that. It locates where two runs diverged, not why, and the scope note concedes the discriminating half is unbuilt. That's fine to ship, but it's "find the first divergence", not "tell me what changed", and the framing should say so.
Happy to re-review once the sub-session handling is fixed — the rest is small and the foundation is good.
Adds
docker agent replay <session-a> <session-b>, which reports the first point where the agentbehaved differently across two recorded runs — the triage answer when a task that worked yesterday
does not work today.
Closes #3947.
--jsonfor scripting,--fail-on-divergencefor CI.The two decisions that make this useful
Tool calls are compared; prose is not. Model output is nondeterministic — two runs of the same
task almost always word things differently while doing exactly the same work. Diffing assistant text
would report a difference on essentially every comparison, which is no signal at all. The tool calls
are what changed the world, so they are what is compared. Assistant text is still carried on each
Turnso a reporter can show what was said around a divergence; it just never decides whether oneoccurred.
ProseIsNeverADivergencepins this.Reporting stops at the first divergence. Once two runs differ, every later turn is downstream of
that difference — different tool results, different context — so comparing the tail produces noise
rather than information. The first divergence is the answer, and the report says so explicitly so
nobody reads the absence of later findings as "nothing else changed".
Tool-call order counts as behaviour: the same set of calls in a different sequence is a different
plan, not a match.
Structure
pkg/replay— pure, no I/O:TurnsOf(*session.Session) []Turn— only assistant messages are turns; user messages are inputs andtool messages are results, neither of which is a decision the model made
Compare(a, b []Turn) Result/CompareSessions(a, b *session.Session) ResultDivergencewith aKindoftool_calls,extra_turn, ormissing_turn, so "diverged mid-run"and "one run kept going" are distinguishable
PrintResult(io.Writer, Result, nameA, nameB)Resultis JSON-tagged and is exactly what--jsonemits.cmd/root/replay.go— flags, store access, and arenderReplayseam that is tested against abytes.Bufferrather than through a live command.Tool arguments are truncated in the report at 120 runes (rune-safe), so a
write_filepayload cannotflood the output — there's a test asserting a 5 KB argument produces under 1 KB of report.
Tests
pkg/replay/replay_test.go(19 tests): identical behaviour despite different prose; prose alonenever diverges; different tool name; same tool with different arguments; order matters; only the
first divergence is reported; both length-mismatch directions; empty runs on both sides;
TurnsOffiltering (user/tool/non-message items excluded, agent name and index preserved); nil session;
CompareSessions; print output for identical, divergent, final-answer, and huge-argument cases; JSONround-trip.
cmd/root/replay_test.go(4 tests): text divergence naming both sides, text identical, JSONround-trip, and flag/arity registration.
Verification
Toolchain
go1.26.5, darwin/arm64.go test ./pkg/replay/ ./cmd/root/golangci-lint run ./pkg/replay/... ./cmd/root/...(v2.12.2, CI's pin)go run ./lint .go build ./...,gofmt -ldocker agent replay --helpgo test ./...pkg/teamloaderfails — pre-existing (Google Cloud ADC), unrelatedScope — read this before reviewing
This is deliberately not the ambitious version of the feature. Re-running a recorded session
against a different model, holding the environment fixed with recorded tool results, is what would
isolate a model or prompt change from environment drift. I did not build that, for a reason worth
stating: the replay diverges the moment the new model calls a tool the recording has no result for, and
the choice between "stop there" (a controlled comparison, less useful) and "execute live from that
point" (useful, no longer controlled) changes what the feature is. That needs a product decision, not
an implementation guess.
Comparing two already-recorded sessions delivers the triage answer today, needs no provider in the
loop, and is the substrate that version would build on —
Compareis the same either way. When Iproposed this in the feature review I flagged it as the speculative one of five, and this scoping is
me acting on that rather than ignoring it.