fix(rewind): truncate by conversation-wide message boundary, invalidate live mirror

[dennisonbertram]

Closes #1370

Summary

Rewinding a multi-run conversation truncated the wrong messages and the live daemon kept serving the pre-rewind history. RestoreRewindPoint compared the rewind point's run-local tool-call step against conversation_messages.step, a conversation-wide message index shared across every run on that conversation, so rewinding to a point in run 2 deleted run 1's later messages too. Separately, the runner's in-memory conversation mirror (populated at each run's completion, served by GET /messages and the next run's context) was never invalidated by rewind, so it kept serving — and the next run kept re-persisting — the pre-rewind history until a daemon restart.

Fix: RewindPoint gained MessageBoundary, the conversation-wide index recorded at capture time of the assistant message that carries the rewound tool call (not just a message count — see the first review follow-up below). RestoreRewindPoint truncates by that boundary when it is recorded, deleting that assistant message and everything after it; points captured before this field existed fall back to the legacy step comparison with a logged warning instead of silently over-deleting. Runner.InvalidateConversationHistory drops the in-memory mirror entry for a conversation; the rewind HTTP handler calls it immediately after a successful restore.

Related: #1303 describes the same resurrection symptom from a different angle (workspace population, TUI JSON tags) and remains open — not touched by this PR.

Review follow-up 1: dangling tool_calls after restore

A reviewer caught that the boundary as first implemented (len(messages) at capture time) pointed just past the assistant message carrying the rewound tool call, so a restore kept that assistant message while deleting only its tool result. Real providers (OpenAI et al.) reject an assistant message with tool_calls that isn't immediately followed by matching tool messages; the fake/stub providers this repo's tests use do not enforce that, so the bug shipped with green tests. Fixed by capturing assistantToolCallIndex — the index of the assistant message itself, right after it's appended in runner_step_engine.go — and using that as MessageBoundary instead. Parallel tool calls issued in one assistant turn all capture the same index, since they share one assistant message. Commits: 3b6e0833 red → e934fbd9 green → 354c0f53 docs.

Review follow-up 2: rewind_points pruning deleted unrelated older points

Found by live verification on main after #1378 merged: RestoreRewindPoint's other truncation query — pruning superseded rewind_points rows — had the same run-local-step bug as message truncation, just not yet fixed. Rewinding to run 2's edit point (step 1 within run 2) deleted run 1's write point too (also step 1 within run 1, an unrelated run), so a later rewind to that still-valid older point returned 404 "not found". Fixed by pruning on MessageBoundary too: a point is superseded only by a strictly greater boundary, or an equal boundary (parallel tool calls sharing one assistant message) captured later (created_at); the target itself is always excluded; falls back to the legacy step predicate only when the target has no recorded boundary. Commits: b0d814a2 red → 2f2a1f58 green → 549ca9b7 docs.

Both follow-ups are folded into this same PR rather than filed as separate issues, since each corrects a value or query this PR itself introduced or was scoped to fix, and neither was ever merged in its original/unfixed form.

Scope and issue reconciliation

In scope per the issue: record a conversation-wide boundary on each rewind point and truncate by it; invalidate the runner's in-memory conversation mirror on rewind. Both are done, refined during review twice (assistant-message index instead of a raw count; the rewind_points prune query keyed on the same boundary). Out of scope per the issue (not touched): crash-safety journaling (#1349), TUI count display (#1303 item 2), snapshot format redesign. #1371's older-point-guard fix (hash refresh across a conversation's snapshots of the same path) is unrelated and already on main, picked up by this branch's rebase; not touched here. No deviations from the issue's stated scope.

Impact analysis reconciliation

• internal/harness/rewind.go: RewindPoint gains MessageBoundary int (zero value = "not recorded", intentionally indistinguishable from omission since a real boundary is always >= 1 messages long in practice).
• internal/harness/conversation_store_sqlite.go: idempotent migration adds rewind_points.message_boundary INTEGER NOT NULL DEFAULT 0; SaveRewindPoint/ListRewindPoints read/write it; RestoreRewindPoint's message-truncation query and its future-point pruning query both branch on MessageBoundary with a log.Printf fallback warning when it's unset.
• internal/harness/runner_step_engine.go: right after the assistant message with tool_calls is appended, captures assistantToolCallIndex := len(messages) - 1; the rewind-point capture site (for each mutating, non-parallel-safe tool call in that turn) sets MessageBoundary: assistantToolCallIndex.
• internal/harness/runner.go: new Runner.InvalidateConversationHistory(conversationID) deletes the conversation from r.conversations/r.conversationTouched/r.conversationMessageWatermarks so the next read falls through to the store.
• internal/server/http_conversations.go: handleRestoreRewind calls the new invalidation method after a successful restore, before responding.
• No API/wire shape change (RewindPoint.MessageBoundary is omitempty and additive), no new routes, no config/env changes. conversations.workspace/tenant handling, approval/permission gating, and file-restore logic are unchanged.
• Every other consumer of RewindPoint/RewindStore (search: runner_step_engine.go, rewind_store_test.go, runner_test.go, http_rewind_test.go) either doesn't read MessageBoundary (unaffected, and correctly exercises the legacy fallback — e.g. bug(rewind): external-modification guard refuses points older than the latest agent edit (stale expected_hash) #1371's own tests) or is one of the tests added/updated here.

Architecture and duplication check

RestoreRewindPoint on SQLiteConversationStore (internal/harness/conversation_store_sqlite.go) is the single owner of both rewind truncation queries (messages and points); Runner.InvalidateConversationHistory is the single owner of in-memory-mirror invalidation, placed next to the existing ConversationMessages/ConversationMessagesSnapshot methods it is meant to keep coherent with. No parallel truncation path, no second conversation-store implementation, no new HTTP route. The rewind HTTP handler (internal/server/http_conversations.go) is the only caller that needed wiring — it already owned the workspace/owner lookup and the RestoreRewindPoint call.

Test-first evidence

Initial fix (red ff24b839 → green db137258 → regression a7a0b6f8):

Red command:

go test ./internal/harness -run 'TestRestoreRewindPoint_MultiRunTruncatesOnlyAfterPoint|TestRestoreRewindPoint_FallsBackWhenBoundaryUnset' -v
go test ./internal/server -run 'TestRestoreRewindEndpoint_MultiRunKeepsPriorMessagesAndInvalidatesLiveMirror' -v

Observed failure:

rewind_store_test.go:178: MessagesTruncated = 7, want 2 (run2's tool result and final answer)
rewind_store_test.go:185: LoadMessages returned 1 messages, want 6 ...
--- FAIL: TestRestoreRewindPoint_MultiRunTruncatesOnlyAfterPoint
rewind_store_test.go:234: expected a logged warning naming the point falling back to step-based truncation, got: ""
--- FAIL: TestRestoreRewindPoint_FallsBackWhenBoundaryUnset
http_rewind_test.go:352: MessagesTruncated = 6, want 2 (run2's tool result and final answer)
http_rewind_test.go:357: after rewind: got 8 messages, want 6 ...
--- FAIL: TestRestoreRewindEndpoint_MultiRunKeepsPriorMessagesAndInvalidatesLiveMirror

This reproduced the exact run-local-step-vs-conversation-wide-index bug from the issue at both the store and HTTP layers, plus the live-mirror-never-invalidated bug (after rewind: got 8, unchanged from before rewind: got 8). Both behavioral, not compile/import errors.

Regression: TestRewindThenNextRunDoesNotResurrectTruncatedMessages (internal/harness/runner_test.go) drives a real three-run Runner flow and proves the run following a rewind doesn't resurrect the truncated tool result/final answer in its LLM request. Confirmed meaningful by temporarily removing the runner.InvalidateConversationHistory(convID) call from the test body alone (implementation untouched): it failed with run3's context resurrected run2's truncated tool result; restoring the call turned it back to PASS.

Review follow-up 1 (red 3b6e0833 → green e934fbd9 → docs 354c0f53):

Red command:

go test ./internal/harness -run 'TestRestoreRewindPoint_NeverLeavesDanglingToolCall' -v

Observed failure:

runner_test.go:496: restore left a dangling assistant message with tool_calls as the last persisted message: {... ToolCalls:[{ID:pa1 ...} {ID:pa2 ...}] ...}
--- FAIL: TestRestoreRewindPoint_NeverLeavesDanglingToolCall

This test drives a real single-turn run with two parallel tool calls in one assistant turn through the actual step-engine capture path and restores using either call's point, asserting: both calls' points share one MessageBoundary, the last persisted message is never an assistant message with tool_calls, and no tool message lacks a preceding assistant tool_calls entry for its ID. The two pre-existing tests above were updated to the corrected semantics (TestRestoreRewindPoint_MultiRunTruncatesOnlyAfterPoint: boundary 6→5, MessagesTruncated 2→3; TestRestoreRewindEndpoint_...: MessagesTruncated 2→3, kept messages 6→5) and both gained the same "no dangling tool_calls" assertion, and both failed for the same behavioral reason before the fix (see commit 3b6e0833's message for the full failure output of all three).

Review follow-up 2 (red b0d814a2 → green 2f2a1f58 → docs 549ca9b7):

Red command:

go test ./internal/harness -run 'TestRestoreRewindPoint_PruneKeepsOlderPointsFromEarlierRuns' -v

Observed failure:

runner_test.go:594: RestoreRewindPoint(write) after an unrelated later-run restore: rewind point "run_...-1-c1" not found
--- FAIL: TestRestoreRewindPoint_PruneKeepsOlderPointsFromEarlierRuns

This test drives two real runs (run 1 writes a.txt, run 2 edits it), restores to run 2's edit point, then restores to run 1's write point — reproducing the exact 404 from the coordinator's live verification. Behavioral, not a compile error.

Green command (all, after both follow-ups):

go test ./internal/harness -run 'TestRestoreRewindPoint_MultiRunTruncatesOnlyAfterPoint|TestRestoreRewindPoint_FallsBackWhenBoundaryUnset|TestRestoreRewindPoint_NeverLeavesDanglingToolCall|TestRestoreRewindPoint_PruneKeepsOlderPointsFromEarlierRuns|TestRewindThenNextRunDoesNotResurrectTruncatedMessages' -v
go test ./internal/server -run 'TestRestoreRewindEndpoint' -v

All PASS.

Verification evidence

Rebased onto latest origin/main twice as it moved during review (final tip 2f3e1609, picking up #1372/#1376/#1373/#1384/#1383/#1382/#1371/#1377/#1381/etc.) with git rebase origin/main. Both rebases conflicted only in docs/logs/engineering-log.md (and once in docs/runbooks/session-rewind.md) — same-day entries/sentences from this branch landing alongside #1371's and #1381's own same-day entries — resolved by keeping both sides' content in every case.

Targeted (all PASS, post-rebase):

go test ./internal/harness -run 'TestRestoreRewindPoint_MultiRunTruncatesOnlyAfterPoint|TestRestoreRewindPoint_FallsBackWhenBoundaryUnset|TestRestoreRewindPoint_NeverLeavesDanglingToolCall|TestRestoreRewindPoint_PruneKeepsOlderPointsFromEarlierRuns|TestRewindThenNextRunDoesNotResurrectTruncatedMessages' -v
go test ./internal/server -run 'TestRestoreRewindEndpoint' -v

Package suites, race, and vet (all PASS, post-rebase):

go test ./internal/harness/... ./internal/server/...      # ok  go-agent-harness/internal/harness  (+6 subpackages ok/cached)
                                                            # ok  go-agent-harness/internal/server
go test ./internal/harness ./internal/server -race        # ok  go-agent-harness/internal/harness  7.897s
                                                            # ok  go-agent-harness/internal/server   17.811s
go vet ./internal/harness/... ./internal/server/...       # clean, exit 0

Full repository package sweep (go test ./internal/..., run twice for reproducibility, prior to the rebases): every package passes except internal/acceptance/ptyrunner, which fails the same way on both runs (PTY did not create completed run for prompt ...). That package has zero import of internal/harness or internal/server and no reference to rewind at all — it drives a real harnesscli binary through a spawned PTY (github.com/creack/pty), which this sandboxed environment doesn't support cleanly. This is a pre-existing, unrelated, environment-dependent failure, not caused by this change.

I did not run the full scripts/test-regression.sh (it runs go test ./... twice plus a coverage gate over the whole repo, including Python-only benchmarks//harness_agent/ dirs that don't build as Go per CLAUDE.md, and would take considerably longer than this fix's scope warrants); the scoped commands above are the smaller regression surface documented for a change of this size.

Real user path: not exercised as a running daemon (no live harnessd/harnesscli session was started for manual verification) — the HTTP-level test drives the real handler (internal/server/http_conversations.go) end-to-end through net/http/httptest, including two real Runner.StartRun calls with a mutating tool, which is the closest available proxy to the issue's live repro without standing up a daemon process.

Rollout and rollback

No migration required beyond the automatic idempotent ALTER TABLE rewind_points ADD COLUMN message_boundary INTEGER NOT NULL DEFAULT 0 that already runs in Migrate() alongside the repo's other incremental column additions — no operator action needed. Existing rewind points recorded before this change have message_boundary=0 and fall back to the previous (imperfect) step-based truncation and pruning with a logged warning, so they remain usable rather than being invalidated. Rollback is a revert of this PR; reverting does not need a down-migration since the added column is additive and unused by anything else. Observability: the fallback path logs via the standard log package naming the point ID and conversation ID whenever a legacy point is restored.

Documentation

• docs/runbooks/session-rewind.md: explains the conversation-wide index of the assistant tool-call message (not a raw message count, and not the run-local step) as the truncation boundary, that pruning of superseded rewind points uses the same boundary, and the live in-memory mirror invalidation.
• docs/logs/engineering-log.md: a 2026-09-05 (Issue #1370 ...) entry plus two same-day follow-up entries documenting the dangling-tool_calls correction and the rewind_points prune correction, following the file's existing per-issue cause/fix/regression format.
• No public API/wire shape change and no new routes, so no OpenAPI/route docs needed updating.

Contract checklist

• Linked issue follows the current structured contract and this PR closes it
• Issue acceptance criteria, impact map, and scope were updated when the design changed — no deviation occurred beyond the two review-caught corrections, both folded into this same PR
• All callers, consumers, sources of truth, and similar abstractions were searched
• No unrelated cleanup, hidden scope growth, duplicated wiring, or parallel abstraction was introduced
• Tests were written first and the expected red failure was observed, or this is a strictly docs-only minor PR
• Targeted checks and the repository-required full regression are green (see caveat on internal/acceptance/ptyrunner, an unrelated pre-existing failure, and on not running the full repo-wide scripts/test-regression.sh)
• Security, compatibility, lifecycle, deployment, observability, documentation, and rollback were reconciled
• Real mouse/keyboard/API/operator behavior was exercised when the change is interaction- or integration-heavy — not exercised against a live daemon; see Verification evidence

🤖 Generated with Claude Code

https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.