Skip to content

fix(chat): preserve history cache correlation - #1130

Draft
bkudiess wants to merge 3 commits into
openclaw:mainfrom
bkudiess:bkudiess-finalize-history-cache-correlation
Draft

fix(chat): preserve history cache correlation#1130
bkudiess wants to merge 3 commits into
openclaw:mainfrom
bkudiess:bkudiess-finalize-history-cache-correlation

Conversation

@bkudiess

@bkudiess bkudiess commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Related to #894

Scope

This PR now contains three focused layers across nine files:

  1. Production history/cache behavior

    • Combine session and thread metadata caches chronologically using cloned entries.
    • Deduplicate only stable (RunId, ToolCallId, LegacyTurn) identities while preserving reused IDs across distinct runs and legacy turns.
    • Merge richer current-canonicalized identity and safe display arguments into the earliest chronological owner.
    • Revalidate cached names, labels, and display arguments through current sanitization on cloned reads without mutating persisted cache entries.
    • Reserve all verified structured history IDs before centrally allocating synthetic history-tool-* IDs for flattened history and unkeyed structured fallbacks.
  2. Regression and renderer proof coverage

    • Prove flattened history never adopts unverified positional cached correlation IDs.
    • Prove history-tool-0 remains a separate interrupted structured row while flattened output is assigned only to synthetic history-tool-1.
    • Exercise the production reducer, activity projection, Reactor renderer, UI Automation tree, and RenderTargetBitmap path in isolated test data with no real Gateway or user settings.
  3. Committed proof artifacts

    • Store the redacted collision screenshot and matching UIA diagnostics under .github/proof/pr-1130/ so this PR head has immutable, reviewable evidence.

hideFromChannelProgress remains intentionally out of scope.

Exact head and base

  • PR head: 128bc29b3adc7263d0ad60d3ef9af285c49078d9
  • Three commits:
    • 3a03dfddbed6323fa5cd970f41b7bb0a0ab2ea0c fix(chat): preserve history cache correlation
    • 683517894bd13dfeeb9996d3934bdac5b4b4863b fix(chat): avoid synthetic history ID collisions
    • 128bc29b3adc7263d0ad60d3ef9af285c49078d9 test(chat): prove history ID collision rendering
  • Fully validated rebase base: 4712e227d110746400e93107c60616544d68f7b4
  • Current main target: 582a44713860409bc34c5acbf6da54a09cf2b10a

main advanced by two commits after the completed validation refresh. Git reports the current head remains cleanly mergeable with the current target, but the validation counts below apply to the exact validated tree at 4712e227 plus this three-commit stack. New-head CI is the current source of truth for the target merge result.

Validation

  • git diff --check: passed
  • .\build.ps1: passed
  • dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 3,631 passed, 0 failed, 32 skipped
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore: 2,255 passed, 0 failed
  • Focused cache/history tests: 30 passed, 0 failed
  • Deterministic cache ordering and non-collapse stress: 25 of 25 iterations, 200 of 200 aggregate tests passed
  • NativeToolIdentityScreenshotProofTests, HistoryCollisionScreenshotProofTests, and ReactorToolActivityProofTests: 3 passed, 0 failed with the runtime-qualified win-x64 command

Real behavior proof

The exact provider regression starts an unfinished structured call with verified ID history-tool-0, then replays flattened tool output. It asserts exactly two rows:

  • Structured history-tool-0: Interrupted, no output.
  • Synthetic history-tool-1: Done, owns flattened output owned by history-tool-1.

The visible proof uses direct production RenderTargetBitmap captures of both production tool cards plus the synthetic output viewport, composed with spacing and without coordinate cropping. Matching UIA assertions verify two distinct visible rows, structured output absence, and synthetic output ownership. The visible fixture proves reducer, projection, and renderer behavior; allocator correctness remains owned by the focused provider regression.

Collision-specific production renderer proof

  • Screenshot repository path: .github/proof/pr-1130/history-collision-68351789.png
    • 1,722 x 330, 43,204 bytes
    • SHA-256 5E0FBB308567AB0F304D6D05DAA202E20C3DBFBFDAB6E571C5D5DED1840C2C6F
  • UIA diagnostics repository path: .github/proof/pr-1130/history-collision-68351789.txt
    • SHA-256 FB29DA6CAB76004E680E0FCAA86C7CFFA8C99600B17D0B9B14665B348397B7E5
  • Captured code-tree provenance: committed head 683517894bd13dfeeb9996d3934bdac5b4b4863b plus exact four-file proof patch SHA-256 F10E5599298252AA5BA0941ABB4B159A6B9545C3BF0D546D05409914429A1BEC; those exact changes are committed by 128bc29b3adc7263d0ad60d3ef9af285c49078d9.

The artifacts were checked for blank media, credential-like content, user-specific paths, and real user/Gateway data before publication.

Review

  • Fresh GPT-5.6 Sol/high independent code review: clean, no actionable findings.
  • Fresh GPT-5.6 Sol/high rubber-duck pass: clean, no actionable blockers.
  • These reviews emitted no numeric confidence score.
  • The bundled isolated autoreview helper could not authenticate because its Codex WebSocket returned 401 Unauthorized; the clean results above are the requested GPT-5.6 Sol/high specialist fallback.
  • No Opus result is claimed here. The coordinator will run the blind Opus/GPT panel against immutable head 128bc29b.
  • The existing ClawSweeper comment, badges, and scores reviewed old head e1be9dfe14065820550d50b684363d2414683fa6. They are stale for this head and are not used as current-head evidence. New-head ClawSweeper is pending.

PR #1111 interaction

PR #1111 remains complementary and unmodified at head be50098bae12e6291ad60b2196ca50996837f94e.

Its semantic patch owns persisted call ID aliases, message-level errors, string-encoded safe arguments, and failed-tool grouping. This PR owns positional cache correlation, flattened row identity, and synthetic/structured ID collision handling. There is no direct changed-line overlap in the source or tests.

Release boundary

#894 is OPEN and remains open until the follow-up lands and a release includes #1078, #1114, and #1130. This PR does not close the issue and intentionally uses Related to #894 rather than a closing keyword.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. labels Aug 8, 2026
@clawsweeper

clawsweeper Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed August 9, 2026, 1:00 PM ET / 17:00 UTC.

ClawSweeper review

What this changes

The PR changes native chat-history replay to preserve cached tool metadata while assigning collision-safe synthetic IDs to flattened tool output.

Merge readiness

⚠️ Ready for maintainer review - 5 items remain

Keep open. The functional patch has strong unit and CI signal, but the prior P2 proof blockers remain: the visible collision fixture bypasses history replay, and its committed artifact records a pre-final commit.

Priority: P2
Reviewed head: 128bc29b3adc7263d0ad60d3ef9af285c49078d9

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The repair and regression coverage are strong, but the visible proof has not yet demonstrated the changed history-replay path on the final head.
Proof confidence 🦐 gold shrimp (3/6) Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🐚 platinum hermit (4/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 6 items Changed correlation path: The PR pre-scans structured history call IDs and allocates synthetic IDs only from unused values before applying replayed entries.
Provider regression coverage: The focused provider test drives structured history ID history-tool-0 plus flattened output through LoadHistoryAsync and expects two distinct rows.
Visible proof does not exercise the changed owner: The collision UI fixture manually applies tool-start and output reducer events, so its screenshot can pass without invoking history replay or the allocator.
Findings 2 actionable findings [P2] Route the collision UI fixture through history replay
[P2] Capture the visual proof from the final commit
Security None None.

How this fits together

Native chat history combines Gateway transcript entries with cached tool metadata, then reduces them into activity rows for the WinUI chat renderer. This PR changes the correlation boundary between history replay and those visible rows.

flowchart LR
A[Gateway history] --> C[History replay]
B[Tool metadata cache] --> C
C --> D[Correlation and ID allocation]
D --> E[Timeline activity rows]
E --> F[WinUI chat renderer]
Loading

Before merge

  • Route the collision UI fixture through history replay (P2) - The prior P2 blocker remains. This fixture constructs both rows with ChatTimelineReducer.Apply, so the screenshot and UIA assertions never invoke OpenClawChatDataProvider.LoadHistoryAsync, the cache matcher, or the new allocator. Keep the focused provider test, but make the visible fixture exercise that path before merge.
  • Capture the visual proof from the final commit (P2) - The committed manifest still records 68351789 as its head and product version. The dirty-tree provenance explains the intervening proof patch, but repository policy requires active changed-state proof from the current PR head; rerun the capture from 128bc29b and update the committed artifact.
  • Resolve merge risk (P1) - Fresh synthetic IDs alter in-memory history correlation, so this remains a session-state-sensitive merge.
  • Resolve merge risk (P1) - The visible artifact does not yet prove the changed provider path from the final commit.
  • Complete next step (P2) - The remaining blockers require contributor-generated final-head behavior proof rather than an automated repair task.

Findings

  • [P2] Route the collision UI fixture through history replay — src/OpenClaw.Tray.WinUI/Pages/ChatPage.xaml.cs:433-460
  • [P2] Capture the visual proof from the final commit — .github/proof/pr-1130/history-collision-68351789.txt:1-3
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +281/-27, tests +808/-14, 1 proof manifest added Most of the 1,102 added lines are regression and renderer-proof coverage, but the production replay path still needs direct visible proof.

Merge-risk options

Maintainer options:

  1. Refresh end-to-end replay proof (recommended)
    Route the UI collision fixture through the provider and recapture the artifact from the final commit before merge.

Technical review

Best possible solution:

Drive the collision scenario through LoadHistoryAsync in the isolated app fixture, then publish redacted UIA and image evidence captured from commit 128bc29.

Do we have a high-confidence way to reproduce the issue?

Yes. The focused provider regression constructs the structured-ID and flattened-output sequence through LoadHistoryAsync, although this read-only review did not execute it.

Is this the best way to solve the issue?

No. The correlation repair is narrow, but the visible proof should use the same provider replay path rather than manually constructed reducer rows.

Full review comments:

  • [P2] Route the collision UI fixture through history replay — src/OpenClaw.Tray.WinUI/Pages/ChatPage.xaml.cs:433-460
    The prior P2 blocker remains. This fixture constructs both rows with ChatTimelineReducer.Apply, so the screenshot and UIA assertions never invoke OpenClawChatDataProvider.LoadHistoryAsync, the cache matcher, or the new allocator. Keep the focused provider test, but make the visible fixture exercise that path before merge.
    Confidence: 0.99
  • [P2] Capture the visual proof from the final commit — .github/proof/pr-1130/history-collision-68351789.txt:1-3
    The committed manifest still records 68351789 as its head and product version. The dirty-tree provenance explains the intervening proof patch, but repository policy requires active changed-state proof from the current PR head; rerun the capture from 128bc29b and update the committed artifact.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 582a44713860.

Labels

Label justifications:

  • P2: This is a bounded chat-history correlation fix with a remaining merge-proof blocker.
  • merge-risk: 🚨 session-state: The patch changes how persisted tool history maps to in-memory activity rows.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

What I checked:

Likely related people:

  • Barbara Kudiess: Introduced the current-main native tool correlation and identity work that this replay change extends. (role: recent correlation feature owner; confidence: high; commits: a1d933a3c7b7, ad472243bcb8; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, tests/OpenClaw.Tray.Tests/OpenClawChatDataProviderTests.cs)
  • Caleb Eden: Recent main history shows ownership of tool activity presentation and history-loading work adjacent to this replay path. (role: recent adjacent contributor; confidence: medium; commits: aac07abb1fbd, 282e583e7257; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, tests/OpenClaw.Tray.Tests/OpenClawChatDataProviderTests.cs)
  • Dallin Romney: Recent main history includes flattened Gateway classification work in the same provider surface. (role: adjacent history-classification contributor; confidence: medium; commits: 23414509b9d0; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Run the collision fixture through LoadHistoryAsync before capturing the renderer.
  • Upload refreshed redacted image and UIA output from commit 128bc29; updating the PR body should trigger re-review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-09T00:06:06.132Z sha 0eb2b21 :: needs changes before merge. :: [P2] Reserve structured IDs before creating synthetic history IDs
  • reviewed 2026-08-09T00:17:56.298Z sha e1be9df :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T00:39:29.322Z sha e1be9df :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T03:16:09.158Z sha e1be9df :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T08:56:27.276Z sha 128bc29 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T10:20:05.602Z sha 128bc29 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-09T11:58:58.931Z sha 128bc29 :: found issues before merge. :: [P2] Exercise history replay in the collision proof
  • reviewed 2026-08-09T14:51:39.574Z sha 128bc29 :: found issues before merge. :: [P2] Route the collision fixture through history replay | [P2] Regenerate the proof from the final PR head

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 9, 2026
bkudiess and others added 2 commits August 8, 2026 20:33
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bkudiess
bkudiess force-pushed the bkudiess-finalize-history-cache-correlation branch from e1be9df to 128bc29 Compare August 9, 2026 08:50
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 9, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant