Skip to content

fix: coalesce GUID-linked URL previews#181

Closed
omarshahine wants to merge 11 commits into
openclaw:mainfrom
omarshahine:fix/url-preview-guid-coalescing
Closed

fix: coalesce GUID-linked URL previews#181
omarshahine wants to merge 11 commits into
openclaw:mainfrom
omarshahine:fix/url-preview-guid-coalescing

Conversation

@omarshahine

@omarshahine omarshahine commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • coalesce Apple URL-preview rows linked to their text row by reply_to_guid, even when the text row omits the URL
  • preserve the existing chat, sender, direction, handle, row-order, time-window, and URL-containment safeguards
  • carry GUID and reply routing through history, watch, stats, and unread paths

Reproduction

On macOS 26.4.1 with imsg 0.12.3, one Messages composition containing text plus https://example.com produced two physical rows. The text row omitted the URL; the URL-preview balloon row contained it and referenced the text row through reply_to_guid. imsg watch emitted two logical events, which caused two downstream OpenClaw turns.

This follows up #141 and #142 and unblocks openclaw/openclaw#91243 and openclaw/openclaw#108436.

Verification

  • red/green regression proof: the GUID-linked history, watch, and stats cases failed before the fix and pass after it (full output)
  • focused URL-preview validation: 53 tests passed
  • DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer make test: 428 tests passed
  • DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer make lint: exit 0 with 10 existing warnings and 0 serious violations
  • git diff --check: passed
  • independent Codex review: no correctness findings

Live after-fix proof

Tested the exact PR head bbbc56b5a4a38e0e9e5d5b2f06fd6bd083ae8e6b on macOS 26.4.1 with a real Messages composition containing a unique text marker plus https://example.com. The URL preview was allowed to render before sending. Competing iMessage consumers were disabled during capture.

Sanitized database evidence for the final send:

Signal Result
Physical Messages rows 2
Tagged text rows 1
URL text rows 1
URL-preview balloon rows 1
Preview payload rows 1
Preview reply_to_guid links to text row GUID 1
Rows with same handle/direction 2/2
Rows in the same chat 2/2
Rows within the coalescing window 2/2

The patched imsg watch --json emitted exactly one logical event for that composition and emitted no delayed second event after a 30-second observation window. The same result was reproduced with a second uniquely tagged composition.

OpenClaw exact head a79a39262782f78b9d399c18b0848242e32fa46d ingested the coalesced event once. A successful model reply is not claimed here: the isolated downstream run first encountered an unrelated legacy workspace migration, then the disposable state lacked the routed agent's auth material. Those OpenClaw environment issues do not affect the direct after-fix imsg row-to-event proof above.

CI note

The current Linux failure is inherited from main, where #176 added an unconditional Darwin import. #182 restores the Linux build and both of its CI jobs are green; rerun this PR after that base repair lands.

@omarshahine

Copy link
Copy Markdown
Contributor Author

Red/green proof

Exact tested head: bbbc56b5a4a38e0e9e5d5b2f06fd6bd083ae8e6b

The regression tests were added first while the implementation still required the originating text row to contain the preview URL.

DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer xcrun swift test --filter 'GUIDLinkedURLPreview|UnlinkedURLPreview'

Before the fix: failed as expected

Exit code: 1

PASS messagesAfterKeepsUnlinkedURLPreviewWhenTextOmitsURL
FAIL messagesAfterCoalescesGUIDLinkedURLPreviewWhenTextOmitsURL
  actual row IDs: [1, 2]
  expected row IDs: [1]
  actual coalesced preview row ID: nil
  expected coalesced preview row ID: 2
FAIL messageWatcherYieldsOneLogicalEventForGUIDLinkedURLPreview
  first event had no coalesced preview
  second event was the separate preview row instead of nil
3 tests run; 4 issues

This reproduces the bug: a GUID-linked Apple preview whose text row omits the URL remains two history rows and two watcher events.

After the fix: passed

Same command and test inputs, exit code: 0

PASS messagesAfterCoalescesGUIDLinkedURLPreviewWhenTextOmitsURL
PASS messagesAfterKeepsUnlinkedURLPreviewWhenTextOmitsURL
PASS messageWatcherYieldsOneLogicalEventForGUIDLinkedURLPreview
3 tests passed

The negative control proves an unlinked same-sender preview is still kept as a separate logical message.

Broader validation

DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer xcrun swift test --filter 'URLPreview|MessageWatcher|messageStats|listChats|schemaDetectsOptionalMessageColumns'
# 53 tests passed

DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer make test
# 428 tests passed

DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer make lint
# exit 0; 10 warnings; 0 serious violations

git diff --check
# exit 0

The focused lane covers native reply_to_guid and non-reaction associated_message_guid routing across history, watch, stats, and unread counts, including prefixed p:0/text-guid normalization.

Live evidence and remaining gate

The pre-fix live reproduction is recorded on the downstream PR: one Messages.app composition produced two physical rows, two imsg watch events, and two OpenClaw turns on imsg 0.12.3: openclaw/openclaw#108436 (comment)

A live resend against this patched imsg head has not been performed. After this fix is merged and released, the final acceptance proof remains: one composition, two structurally linked physical rows, exactly one imsg watch event, and exactly one OpenClaw turn.

@clawsweeper

clawsweeper Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 18, 2026, 7:13 PM ET / 23:13 UTC.

Summary
The branch coalesces GUID-linked Apple URL-preview rows across history, search, stats, unread counts, and watch delivery, while adding a watch-only cursor for resume safety and subscribing before contact discovery.

Reproducibility: yes. for the original duplicate-delivery bug: the supplied fixture and live Messages evidence establish two GUID-linked physical rows that should produce one logical event. The remaining concern is source-reproducible from the changed public resume contract: persist a folded event's legacy id, reconnect with --since-rowid, and check for replay or altered boundary behavior.

Review metrics: 2 noteworthy metrics.

  • Patch surface: 32 files; 2,079 added, 119 removed. The fix spans read paths, watcher delivery, public JSON output, documentation, and regression coverage, so upgrade behavior needs focused review.
  • Resume-facing docs: 4 documentation files changed. The branch explicitly revises the public resume guidance from id to cursor, making compatibility a user-facing merge decision.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🦐 gold shrimp
Result: blocked until stronger real behavior proof is added.

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

Rank-up moves:

  • [P1] Decide and implement the id-resume compatibility policy.
  • Attach redacted exact-head watch output covering delayed preview coalescing plus legacy-id and cursor reconnects.
  • Wait for the current macOS check to complete against the exact head.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: Maintainer live proof and hosted CI cover prior head 1c8bf04…, but the latest 5d867020… head changes startup subscription ordering; add redacted exact-head terminal or live output showing one delayed-preview event and reconnect behavior using both legacy id and new cursor. Redact phone numbers, chat identifiers, and other private data; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Existing watch consumers were documented to persist id for --since-rowid; after a folded preview, resuming from that legacy value can replay or otherwise alter delivery at the folded physical-row boundary unless the implementation retains a legacy compatibility path.
  • [P1] The latest head adds a startup delivery-order change after the prior live proof, while the current macOS check is still in progress; merge should wait for exact-head runtime evidence covering both delayed-preview folding and a resume/reconnect path.

Maintainer options:

  1. Add a legacy resume compatibility path (recommended)
    Preserve documented id-based resume semantics around folded preview rows, add focused regression coverage, and attach exact-head redacted live resume proof before merge.
  2. Deliberately ship the cursor migration
    Accept the compatibility break only with explicit release-owner approval, a migration notice for downstream consumers, and exact-head proof of the intended at-least-once behavior.
  3. Pause the public cursor change
    Keep the GUID-preview coalescing fix but defer the resume-contract redesign until the repository has an approved compatibility plan.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Preserve legacy id-based --since-rowid behavior for folded URL previews; add a regression test and attach exact-head redacted watch resume proof for both id and cursor paths.

Next step before merge

  • [P1] A repository owner must choose the public watch-resume compatibility policy; the remaining exact-head proof also cannot be supplied by an automated repair lane.

Maintainer decision needed

  • Question: Should imsg watch preserve the documented id-based --since-rowid contract for existing consumers, or intentionally require all consumers to migrate to the new cursor contract?
  • Rationale: The PR changes an established public CLI/JSON resume contract. The implementation can support either direction, but choosing an upgrade-breaking API transition versus a compatibility layer requires repository-owner intent.
  • Likely owner: steipete — They authored the original coalescing behavior, the new cursor/delivery changes, and the prior maintainer verification.
  • Options:
    • Preserve legacy id resume compatibility (recommended): Teach resume handling to recognize a folded message's completion boundary when given its legacy logical id, while documenting cursor as the preferred durable token for new clients.
    • Adopt the cursor-only contract: Accept the behavior change, explicitly version and announce the migration, and require proof that supported consumers are updated before release.

Security
Cleared: The diff changes local SQLite read/coalescing, watcher delivery, CLI output, tests, and docs; it introduces no dependency, workflow, secret, permission, or supply-chain change.

Review findings

  • [P1] Preserve the documented id resume contract — Sources/imsg/OutputModels.swift:82
Review details

Best possible solution:

Retain id as a backward-compatible resume input for existing watch clients, introduce cursor as the preferred forward path, and prove on the exact head that legacy and cursor-based reconnects neither skip nor create an extra logical delivery.

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

Yes for the original duplicate-delivery bug: the supplied fixture and live Messages evidence establish two GUID-linked physical rows that should produce one logical event. The remaining concern is source-reproducible from the changed public resume contract: persist a folded event's legacy id, reconnect with --since-rowid, and check for replay or altered boundary behavior.

Is this the best way to solve the issue?

No, not yet: GUID-based structural coalescing is the right narrow fix, but replacing the documented id resume contract with cursor without a compatibility decision is not the safest upgrade path. A legacy-compatible reader plus cursor as the preferred new token is the narrower maintainable solution.

Full review comments:

  • [P1] Preserve the documented id resume contract — Sources/imsg/OutputModels.swift:82
    Existing consumers were told to persist id as the exclusive --since-rowid token. A folded preview now has a different completion cursor, so reconnecting from the legacy id can replay or change logical delivery at that boundary. Preserve legacy input semantics or obtain an explicit approved migration and prove both resume paths before merge.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add merge-risk: 🚨 compatibility: The branch changes the documented --since-rowid resume token from id to a new watch-only cursor field.

Label justifications:

  • P1: Duplicate or replayed watch deliveries can create duplicate downstream agent turns for active users.
  • merge-risk: 🚨 compatibility: The branch changes the documented --since-rowid resume token from id to a new watch-only cursor field.
  • merge-risk: 🚨 message-delivery: Folded preview boundaries and reconnect behavior can replay, skip, or split logical watch events if resume handling is not backward-compatible.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Maintainer live proof and hosted CI cover prior head 1c8bf04…, but the latest 5d867020… head changes startup subscription ordering; add redacted exact-head terminal or live output showing one delayed-preview event and reconnect behavior using both legacy id and new cursor. Redact phone numbers, chat identifiers, and other private data; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Existing documented resume contract: The base documentation says --since-rowid resumes from a stored message id; this is the compatibility contract existing consumers use. (README.md:235, ca91c41b47e0)
  • New cursor contract: The PR changes the public JSON payload to add a separate cursor and documents that consumers must persist it rather than id, which changes upgrade behavior for watch clients. (Sources/imsg/OutputModels.swift:82, 5d86702043da)
  • Current-head startup behavior: The latest commit moves stream subscription ahead of asynchronous contact discovery and adds coverage for that ordering; it is a meaningful behavior change after the prior maintainer-verified head. (Sources/imsg/Commands/WatchCommand.swift:96, 5d86702043da)
  • Prior maintainer verification: A collaborator reported live CLI fixture proof, 84 focused regression tests, build/lint success, and hosted CI success at the prior head, but that verification names 1c8bf04…, not the latest 5d867020… head. (1c8bf04bf9ef)
  • Feature-history ownership: The original URL-preview coalescing implementation was merged through the earlier PR, and the current branch contains multiple follow-up commits by the same recent area contributor, including the latest head. (Sources/IMsgCore/MessageWatcher.swift:1, 1c5160947c92)

Likely related people:

  • steipete: Merged the initial URL-preview coalescing work and authored the current branch's hardening, cursor, delivery-boundary, search, and startup-order commits. (role: original feature author and recent area contributor; confidence: high; commits: 1c5160947c92, 428427bd9d62, 29bfdd7eab3a; files: Sources/IMsgCore/MessageWatcher.swift, Sources/IMsgCore/MessageStore+URLPreviews.swift, Sources/imsg/Commands/WatchCommand.swift)
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.
Review history (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-17T03:46:56.276Z sha bbbc56b :: needs real behavior proof before merge. :: [P3] Revert the release-owned changelog edit
  • reviewed 2026-07-17T04:06:49.895Z sha bbbc56b :: needs real behavior proof before merge. :: [P3] Revert the release-owned changelog edit
  • reviewed 2026-07-17T05:38:23.204Z sha bbbc56b :: needs changes before merge. :: [P3] Revert the release-owned changelog edit
  • reviewed 2026-07-17T06:20:01.189Z sha bbbc56b :: needs changes before merge. :: [P3] Revert the release-owned changelog edit
  • reviewed 2026-07-18T06:13:59.659Z sha f825c71 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T06:20:12.206Z sha 6ec798c :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T07:02:02.707Z sha 6ec798c :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T22:49:35.449Z sha 1c8bf04 :: needs real behavior proof before merge. :: [P1] Preserve the documented id resume contract | [P3] Remove the release-owned changelog entry

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 17, 2026
@omarshahine

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 17, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@omarshahine

Copy link
Copy Markdown
Contributor Author

Live after-fix proof

Tested exact head bbbc56b5a4a38e0e9e5d5b2f06fd6bd083ae8e6b on macOS 26.4.1 using a real Messages composition with a unique marker and https://example.com, sent only after the URL preview rendered. Competing iMessage consumers were disabled during capture.

Sanitized database result: 2 physical rows in 1 chat; 1 tagged text row; 1 URL-preview balloon row containing the URL and payload; the preview row's reply_to_guid matched the text row GUID; both rows had the same handle and direction and were inside the coalescing window.

Patched imsg watch --json emitted exactly 1 logical event and no delayed second event during a 30-second observation window. A second uniquely tagged composition reproduced the same one-event behavior.

OpenClaw exact head a79a39262782f78b9d399c18b0848242e32fa46d ingested the coalesced event once. I am not claiming a successful model reply: the isolated downstream environment separately hit legacy workspace/auth setup issues. The direct live Messages-to-imsg watch proof is complete.

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 17, 2026
@omarshahine
omarshahine force-pushed the fix/url-preview-guid-coalescing branch from bbbc56b to f825c71 Compare July 18, 2026 06:11
@omarshahine

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 18, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient Contributor real behavior proof is sufficient. 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 Jul 18, 2026
@omarshahine

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 18, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot removed the rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. label Jul 18, 2026
@clawsweeper clawsweeper Bot added the rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. label Jul 18, 2026
steipete and others added 7 commits July 18, 2026 13:25
Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
@steipete

steipete commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification at exact head 5d86702043daae2ccf65ab8ce9a79039eab45313:

  • Focused URL-preview/history/search/stats/unread/watch regression suite: 85 tests passed, including command-layer proof that watch subscribes before asynchronous contact discovery.
  • make lint: passed (0 serious violations).
  • make build: passed; universal CLI and bridge helper built and signature verification passed.
  • Autoreview: no accepted or actionable findings. One BB-event-mode finding was rejected because --bb-events is additive and the normal stream is consumed immediately below that task.
  • Exact-head hosted CI: macOS lint/test/build and Linux read-core test/build passed in run 29664759652.
  • Live proof lineage: the real Messages.app proof on the earlier coalescing head observed one watch event and no delayed duplicate; signed head 1c8bf04 also emitted one coalesced logical message from the synthetic two-row database. The exact 5d86702 signed invocation is blocked before database output by this host's Contacts TCC preflight; a process sample shows CNContactStore initialization waiting on tccd. I am not claiming exact-head live output through that blocked boundary.

This is land-ready from code review and exact-head CI. I have intentionally not merged it.

Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com>
@steipete

Copy link
Copy Markdown
Collaborator

Closing after binary maintainer review. The bug is real and the repaired head is CI-clean, but it changes the documented watch resume contract from id to a new physical cursor. Exact-head delayed-preview reconnect proof for both legacy id and cursor is also unavailable because the signed CLI is blocked in this host's Contacts/TCC preflight.

Landing therefore requires an owner decision on legacy id compatibility plus exact-head resume proof. We do not leave contributor PRs in a change-request limbo; the decision brief and repair SHA are preserved for a future focused implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants