Repository navigation
fix: delete messages outside the loaded transcript - #379
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. What this changesDelete all loaded transcript parts of a message, loading older messages when necessary and refusing unsupported targets before dispatch. Example: Delete photo-and-caption message FF938A80… with zero newer messages on macOS 26.6.2.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review This PR fixes a demonstrated deletion failure still present on main; the earlier diagnostic finding is resolved, and no actionable defect remains. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherCLI and RPC deletion requests reach the injected Messages helper, which resolves the chat and message, obtains its transcript parts, and submits them to IMCore for recoverable deletion. flowchart LR
A[CLI or RPC request] --> B[Injected helper]
B --> C[Resolve chat and message]
C --> D[Find or load transcript parts]
D --> E[Reject unsupported targets]
E --> F[IMCore recoverable deletion]
Technical reviewBest possible solution: Land the focused transcript-item repair while preserving Recently Deleted behavior and explicit pre-dispatch refusals. Do we have a high-confidence way to reproduce the issue? Main's single-item deletion path and the contributor's paired real CLI runs establish the failure; this read-only review ran no target code. Is this the best way to solve the issue? The patch addresses the required IMCore input directly, reuses existing lookup helpers, and preserves recoverable deletion instead of switching to permanent deletion. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5af52a029b3d. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed November 3, 2026, 7:25 AM ET / 12:25 UTC (Revision 2). |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
status: 👀 ready for maintainer look
Load older messages into the transcript before deleting every part, and refuse unsupported reactions and unsent placeholders before dispatch (openclaw#379, thanks @logananderson). Co-authored-by: Logan Anderson <poggus@gmail.com>
7d66994 to
c03bae2
Compare
What
delete-messagereturnsqueuedbut deletes nothing for most messages.-[IMChat deleteChatItems:]only acts on the chat's own loaded transcript items, which cover only the most recent messages, and silently ignores anything else. For an older message,findMessageItembuilds a new chat item fromIMChatHistoryController loadMessageWithGUID:, the delete ignores it, and the helper still reports success. Photos with a caption, tapbacks, and unsent messages don't delete either.This change keeps the existing
deleteChatItems:call and Recently Deleted behavior. It changes what gets passed in, and when to refuse:loadMessagesBeforeAndAfterGUID:numberOfMessagesToLoadBeforeGUID:numberOfMessagesToLoadAfterGUID:loadImmediately:threadIdentifier:(inside@try) and waits up to 2 s for it.deleteChatItems:.Behavior changes:
queued, nothing deleted).IMAssociatedMessageItem):not_started(wasqueued, nothing deleted).IMMessageRetractionChatItem):not_started(wasqueued, nothing deleted).not_started, "Couldn't load this message in Messages; open the conversation in Messages and delete it there", instead of a false success.Message not foundis nownot_started, like the other refusals before dispatch.Loading moves the chat's loaded window, so a Messages window showing that chat may jump.
deleteIMMessageItems:also exists and deletes permanently, without Recently Deleted. It's left out to keep delete's current semantics.docs/bridge.mdanddocs/rpc.mddescribe the behavior.Tests
Tests/IMsgHelperTests/DeleteMessageTests.mrunshandleDeleteMessageagainst a fakeIMChatthat does what IMChat did in the live runs:deleteChatItems:only removes loaded transcript items, a rebuilt item for a text message still deletes it, one for a photo with a caption deletes nothing, and unsent placeholders never delete. It covers:Failing first on main (
5af52a0); the recent message passes there, as it does live:All pass on this branch.
Live proof
Release
imsgCLI and helper builds of main (5af52a0) and this branch (0478410; the later7d66994only rewords the load-failure error, which no live run reaches), swapped in as the injected helper and run on the same messages in one chat.in_chatcomes fromchat_message_join, andrecently_deletedfromchat_recoverable_message_join. "Newer" is the number of later messages in the chat at the time of each delete; it drops between rows and runs because earlier deletes removed newer messages. Phone numbers are replaced and GUIDs shortened.queued, still in chatqueued, still in chatqueued, still in chatqueued, still in chatqueued, still in chatnot_started, still in chatqueued, still in chatnot_started, still in chatThe "thread companions" lines at the end of each log are the messages that must survive. On 15.6.1,
5E62202Cis the deleted reply's thread parent and4DD65153is the deleted parent's reply. On 26.7.2,307FFBFEis the deleted reply's thread parent,B7CF328Cis another reply in that thread, and39BB0E39is the deleted parent's reply.A photo with a caption fails on main even when it's the newest message. On a 26.6.2 (25G83) VM, the same message, with 0 newer messages:
main:
this branch:
macOS 15.6.1 (24G90), main
macOS 15.6.1 (24G90), this branch
macOS 26.7.2 (25G332), main
macOS 26.7.2 (25G332), this branch
Checks
At
7d66994:Not run: the Linux gate (no Docker here).
Maintainer follow-up
Added a failing nested-reply regression and fixed transcript selection to require each entry’s own backing-message GUID. Deleting a reply cannot select its containing parent. The regression verifies refusal without an own entry and correct deletion with the parent preserved.
Focused synthetic regressions and independent P0–P2 review pass. Live Messages sends were not used during this round.