Skip to content

[Repo Assist] fix(chat): persist attachment metadata in sidecar for history reload#615

Draft
github-actions[bot] wants to merge 1 commit into
masterfrom
repo-assist/fix-issue-453-attachment-sidecar-855965357507df28
Draft

[Repo Assist] fix(chat): persist attachment metadata in sidecar for history reload#615
github-actions[bot] wants to merge 1 commit into
masterfrom
repo-assist/fix-issue-453-attachment-sidecar-855965357507df28

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot commented Jun 1, 2026

🤖 This is an automated pull request from Repo Assist.

Closes #453

Summary

Attachment chips (file/image indicators) in the chat timeline disappear after a history reload, session switch, or app restart. This PR fixes the root cause and adds 5 new tests covering the persistence and rehydration path.

Root Cause

SendMessageAsync appends zero-width-space chip markers (\u200B📎 filename) to the local display text, but only the trimmed text (without markers) is sent to the gateway. On history reload, the gateway returns the trimmed text only — attachment chips are lost.

Fix

OpenClawChatDataProvider.cs

  • Add attachment-sidecar.json (written alongside tool-metadata.json) storing per-thread attachment display metadata: filename, IsImage flag, trimmed text, and send timestamp.
  • In SendMessageAsync: when attachments are present, record a sidecar entry before dispatching to the gateway.
  • In LoadHistoryAsync: for each role=user message, call HydrateAttachmentsFromSidecarLocked — matches by text equality + 24-hour timestamp window, then re-injects \u200B📎 / \u200B🖼️ chip markers.
  • In DisposeAsync: flush sidecar to disk.
  • Sidecar is capped at 20 threads × 200 entries; oldest evicted first.

Spoof protection: A message that looks like chip marker text but has no matching sidecar entry gets no additional chips injected.

Trade-offs

  • Only filename and image-flag are persisted (no bytes), so image thumbnails revert to chips after restart (same as non-image files). Full image persistence would be multi-MB on disk per image — not appropriate here.
  • Timestamp matching uses a generous 24-hour window; same message text sent multiple times within 24h matches the closest timestamp.

Test Status

OpenClaw.Tray.TestsGITHUB_ENV env-file infrastructure failure is pre-existing; tests still run and pass.

Passed! - Failed: 0, Passed: 879, Skipped: 2, Total: 881

New tests all pass:

  • SendMessageAsync_WithAttachment_RecordedInSidecar
  • SendMessageAsync_WithImageAttachment_RecordedAsImageInSidecar
  • LoadHistoryAsync_WithMatchingSidecarEntry_InjectsAttachmentChips
  • LoadHistoryAsync_WithNoSidecarEntry_DoesNotModifyText
  • HydrateAttachmentsFromSidecarLocked_PastedMarkerText_NotHydrated

Also isolated CreateProvider to a per-test temp directory, preventing sidecar/cache cross-contamination between tests.

OpenClaw.Shared.Tests: 8 pre-existing failures unrelated to this change (path resolution, MCP host tests).

Generated by 🌈 Repo Assist, see workflow run. Learn more.

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@97143ac59cb3a13ef2a77581f929f06719c7402a

Closes #453

Root cause: when SendMessageAsync sends a message with attachments, the
display text is built locally with zero-width-space chip markers but only
the trimmed text (without markers) is sent to the gateway. On history
reload or app restart, the gateway returns only the trimmed text; the
chip markers are gone and attachment chips disappear.

Fix:
- Add attachment-sidecar.json (alongside tool-metadata.json) that
  persists per-thread attachment display metadata (filename + isImage)
  keyed by trimmed text and send timestamp.
- On SendMessageAsync, when attachments are present, record a sidecar
  entry before sending.
- On LoadHistoryAsync, for each user message, match against the sidecar
  by text + 24-hour timestamp window and re-inject chip markers.
- Pasted/spoofed markers without a sidecar entry are not affected.

Tests added:
- SendMessageAsync_WithAttachment_RecordedInSidecar
- SendMessageAsync_WithImageAttachment_RecordedAsImageInSidecar
- LoadHistoryAsync_WithMatchingSidecarEntry_InjectsAttachmentChips
- LoadHistoryAsync_WithNoSidecarEntry_DoesNotModifyText
- HydrateAttachmentsFromSidecarLocked_PastedMarkerText_NotHydrated

Also isolate CreateProvider to per-test temp directory to prevent
cache cross-contamination between tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

Codex review: needs changes before merge. Reviewed May 31, 2026, 9:55 PM ET / 01:55 UTC.

Summary
This PR adds an attachment-sidecar JSON cache to OpenClawChatDataProvider and tests so history reloads can re-inject attachment chips for sent user messages.

Reproducibility: yes. Source inspection shows the underlying bug path: attachments render from local chip text while history reloads rebuild from trimmed gateway text, and the patch findings are directly source-reproducible from the added sidecar write and hydrate paths.

Review metrics: 2 noteworthy metrics.

  • Changed files: 2 modified. The diff is concentrated in the chat data provider and its unit tests.
  • New persisted store: 1 JSON sidecar added. The new local cache changes the chat data retained on disk, so maintainers should review data minimization before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🌊 off-meta tidepool
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Replace plaintext TrimmedText persistence with a non-reversible match key or stable message id and add a sidecar-content regression test.
  • [P2] Move sidecar commit after successful gateway send or remove failed entries, then add a failed-send rehydration regression test.

Mantis proof suggestion
After the blockers are repaired, a short visible proof of attachment chips surviving history reload or restart would materially help review. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify that a sent chat attachment still renders as an attachment chip after history reload and app restart, with private filenames/content redacted.

Risk before merge

  • [P1] Merging as-is would create a new plaintext local cache of user message text for attachment sends, not only attachment metadata.
  • [P1] Because sidecar entries are saved before SendChatMessageAsync succeeds, a failed attachment send can later be matched to an unrelated same-text history message and show an attachment that was never sent.

Maintainer options:

  1. Repair sidecar persistence before merge (recommended)
    Store a non-reversible match key instead of plaintext TrimmedText, only retain entries for successful sends, and add regression tests for both cases.
  2. Pause for protocol direction
    If maintainers prefer gateway-provided structured attachment metadata over a local sidecar, pause this PR and route that design decision back to the linked issue.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Update the attachment sidecar so it does not serialize full user message text, using a non-reversible match key or stable history id instead; ensure sidecar entries are committed only after SendChatMessageAsync succeeds or are removed on failure; add tests proving attachment-sidecar.json does not contain message content and that a failed attachment send does not hydrate a later same-text no-attachment history message.

Next step before merge

  • [P2] A focused repair can update the sidecar to avoid plaintext content and failed-send staleness, with tests in the existing provider test file.

Security
Needs attention: The diff introduces a new local persistence path that stores user message text in plaintext.

Review findings

  • [P1] Avoid persisting full message text in the sidecar — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:236
  • [P2] Only persist sidecar entries after a successful send — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:250-252
Review details

Best possible solution:

Keep the sidecar concept, but store only minimized attachment metadata plus a non-reversible match key or stable message id, and commit sidecar entries only for sends accepted by the gateway.

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

Yes. Source inspection shows the underlying bug path: attachments render from local chip text while history reloads rebuild from trimmed gateway text, and the patch findings are directly source-reproducible from the added sidecar write and hydrate paths.

Is this the best way to solve the issue?

No. A local sidecar is a reasonable narrow direction, but this implementation should not persist plaintext message content or save metadata before the gateway accepts the send.

Full review comments:

  • [P1] Avoid persisting full message text in the sidecar — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:236
    This stores the full user message in attachment-sidecar.json for every attachment send. The sidecar only needs a match key plus attachment metadata, so this adds a new plaintext local prompt cache and can expose sensitive message content on disk; store a non-reversible key or stable history id instead.
    Confidence: 0.9
  • [P2] Only persist sidecar entries after a successful send — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:250-252
    The sidecar entry is added and flushed before SendChatMessageAsync succeeds. If the gateway call throws, the failed attachment record remains and LoadHistoryAsync can attach it to a later same-text message within the 24-hour window, showing an attachment that was never sent.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 6d14b00f6cfc.

Label changes

Label changes:

  • add P2: This is a bounded chat attachment persistence bug fix, but the current PR has repairable merge blockers rather than emergency impact.
  • add merge-risk: 🚨 session-state: Failed-send sidecar records can be re-associated with later same-text history messages, corrupting reconstructed chat attachment state.
  • add merge-risk: 🚨 security-boundary: The diff would persist full user message text for attachment sends in a new local JSON file.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This is a github-actions bot Repo Assist PR, so the external contributor proof gate is not applied; the PR body reports unit test output only.

Label justifications:

  • P2: This is a bounded chat attachment persistence bug fix, but the current PR has repairable merge blockers rather than emergency impact.
  • merge-risk: 🚨 security-boundary: The diff would persist full user message text for attachment sends in a new local JSON file.
  • merge-risk: 🚨 session-state: Failed-send sidecar records can be re-associated with later same-text history messages, corrupting reconstructed chat attachment state.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This is a github-actions bot Repo Assist PR, so the external contributor proof gate is not applied; the PR body reports unit test output only.
Evidence reviewed

Security concerns:

  • [medium] Plaintext prompt text in attachment sidecar — src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:236
    attachment-sidecar.json serializes TrimmedText for every attachment send, which retains user message content locally even though the sidecar only needs attachment metadata and a match key.
    Confidence: 0.9

Acceptance criteria:

  • [P1] ./build.ps1.
  • [P1] dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore.

What I checked:

Likely related people:

  • Ranjesh: Git blame shows the current attachment send path, image preview cache, timeline parser, and chat history model in the relevant files coming from commit 9de9b5b. (role: introduced current behavior; confidence: high; commits: 9de9b5ba0f8a; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs, src/OpenClaw.Shared/Models.cs)
  • christineyan4: The linked issue discussion explicitly tagged christineyan4 as the requested follow-up owner for this attachment metadata work. (role: requested follow-up owner; confidence: medium; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs)
  • shanselman: shanselman filed the linked attachment persistence issue and added the follow-up owner context that shaped the requested fix boundary. (role: reporter and review-context provider; confidence: medium; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, tests/OpenClaw.Tray.Tests/OpenClawChatDataProviderTests.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automation merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. 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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. repo-assist 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.

Persist chat attachment metadata across history reloads

0 participants