Skip to content

fix: clear cross-run messaging-tool sent-text state after every compaction#80285

Open
jetd1 wants to merge 1 commit into
openclaw:mainfrom
jetd1:fix/cross-run-messaging-dedupe-leak
Open

fix: clear cross-run messaging-tool sent-text state after every compaction#80285
jetd1 wants to merge 1 commit into
openclaw:mainfrom
jetd1:fix/cross-run-messaging-dedupe-leak

Conversation

@jetd1

@jetd1 jetd1 commented May 10, 2026

Copy link
Copy Markdown
Contributor

Problem

When a run completes and the session compacts, the arrays tracking texts sent via messaging tools (message(action=send), sessions_send, etc.) were only cleared on compaction retries (willRetry=true). On normal compaction (willRetry=false), stale entries survived into the next run.

isMessagingToolDuplicateNormalized() compares the final reply text against these stale entries using substring matching (normalized.includes(normalizedSent)). If the new run's text happens to share a normalized substring with any entry from the previous run, the block reply is silently dropped with:

Skipping message_end block reply - already sent via messaging tool

Fix

Clear messagingToolSentTexts, messagingToolSentTextsNormalized, messagingToolSentTargets, and messagingToolSentMediaUrls unconditionally in the compaction_end handler — before the willRetry branch — so the next run always starts with a clean slate.

Verification

  • Relevant tests pass (6/6) in pi-embedded-subscribe.handlers.compaction.test.ts (updated fixture)
  • No logic change to emitBlockChunk, handleMessageEnd, or the dedup function itself — change is purely about state lifecycle between runs

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 10, 2026
@clawsweeper

clawsweeper Bot commented May 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 11:46 PM ET / 03:46 UTC.

Summary
The PR adds unconditional clearing of messaging-tool sent-text, target, and media-URL tracking in the legacy pi-embedded compaction-end handler and initializes those arrays in its test fixture.

PR surface: Source +10, Tests +4. Total +14 across 2 files.

Reproducibility: yes. at source level but not as a live run in this review. Current main records messaging-tool sent text, uses the normalized array for duplicate suppression, and only clears those arrays during compaction retry reset, not after normal compaction.

Review metrics: 1 noteworthy metric.

  • Pre-retry cleanup placement: 1 cleanup block added before willRetry handling. The placement matters because the current retry path records committed messaging delivery evidence before clearing these arrays.

Root-cause cluster
Relationship: canonical
Canonical: #80285
Summary: This PR is the focused proposed work for cross-run messaging-tool sent-text state leakage after compaction; the linked sessions-send compaction replay issue and PR are adjacent but distinct.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until 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:

  • Port the cleanup to the current embedded-agent-subscribe compaction handler after retry evidence capture.
  • [P1] Add retry and non-retry regression coverage for the sent-state cleanup lifecycle.
  • [P1] Add redacted real behavior proof; screenshots or recordings are fine when visible, and terminal output, copied live output, linked artifacts, or redacted logs also count.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists targeted tests, but it does not include redacted terminal output, logs, screenshot, recording, copied live output, or a linked artifact proving the real compaction plus messaging-tool path after the fix. 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.

Mantis proof suggestion
A real compaction plus messaging-tool send proof would materially show that the next run's final reply is not suppressed and retry delivery evidence remains intact. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify compaction followed by message(action=send) or sessions_send does not suppress the next run's final reply and preserves retry delivery evidence.

Risk before merge

  • [P1] The branch is conflicting and edits removed legacy files, so merging as-is cannot update the active compaction runtime.
  • [P1] A direct port of the added placement before willRetry handling can clear committed messaging delivery evidence before retry terminal/liveness state consumes it.
  • [P1] The external PR body lists targeted tests but does not include redacted after-fix proof from a real compaction plus messaging-tool delivery path.

Maintainer options:

  1. Port Cleanup After Evidence Capture (recommended)
    Move the cleanup into the current embedded-agent compaction handler so normal compaction starts the next run clean while retry compaction records delivery evidence before clearing state.
  2. Require Current-Path Real Proof
    Keep the PR blocked until the contributor posts redacted terminal output, logs, a recording, screenshot, copied live output, or linked artifact proving the current compaction plus messaging-tool path.
  3. Replace The Stale Branch
    If maintainers do not want to salvage the conflicting branch, open a narrow replacement that preserves the lifecycle fix and credits this PR before closing it.

Next step before merge

  • [P1] Human handling is needed because the external PR lacks real behavior proof and needs a current-path port with retry evidence ordering preserved.

Security
Cleared: The diff only changes agent lifecycle state cleanup and a test fixture; no concrete security or supply-chain concern was found.

Review findings

  • [P1] Port cleanup after retry evidence capture — src/agents/pi-embedded-subscribe.handlers.compaction.ts:90-93
Review details

Best possible solution:

Port the cleanup to the current embedded-agent compaction lifecycle after retry delivery evidence is captured, add retry and non-retry regression coverage, and require redacted real behavior proof before merge.

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

Yes, at source level but not as a live run in this review. Current main records messaging-tool sent text, uses the normalized array for duplicate suppression, and only clears those arrays during compaction retry reset, not after normal compaction.

Is this the best way to solve the issue?

No. The cleanup idea is plausible, but the submitted branch targets removed pi-embedded files and clears before retry evidence is preserved; the safer fix belongs in the current handler after evidence consumers run.

Full review comments:

  • [P1] Port cleanup after retry evidence capture — src/agents/pi-embedded-subscribe.handlers.compaction.ts:90-93
    This cleanup is added to the old pi-embedded compaction handler, but current main no longer contains that file and uses embedded-agent-subscribe.handlers.compaction.ts. When porting, do not keep this before-willRetry placement: the current retry path first snapshots committed messaging delivery evidence, so clearing these arrays earlier can make retry terminal/liveness handling miss a real side effect.
    Confidence: 0.93

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority compaction/session-state bug fix with bounded but real message-delivery impact.
  • merge-risk: 🚨 session-state: The diff changes compaction lifecycle state that also feeds retry and terminal state handling.
  • merge-risk: 🚨 message-delivery: The touched sent-text arrays decide whether final replies are suppressed as already delivered messaging-tool output.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists targeted tests, but it does not include redacted terminal output, logs, screenshot, recording, copied live output, or a linked artifact proving the real compaction plus messaging-tool path after the fix. 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

PR surface:

Source +10, Tests +4. Total +14 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 10 0 +10
Tests 1 4 0 +4
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 14 0 +14

What I checked:

Likely related people:

  • steipete: Authored and merged the runtime internalization PR that introduced the active embedded-agent compaction handler and messaging-dedupe helper, plus adjacent normalization/helper docs work. (role: feature-history contributor; confidence: high; commits: bb46b79d3c14, 00d8d7ead059, dbfe5a252cd2; files: src/agents/embedded-agent-subscribe.handlers.compaction.ts, src/agents/embedded-agent-subscribe.ts, src/agents/embedded-agent-helpers/messaging-dedupe.ts)
  • vincentkoc: Recent work touched compaction lifecycle and final-reply identity near the same session-state and message-delivery boundary. (role: recent area contributor; confidence: medium; commits: 599abac902f7, 756e9d16a0a6; files: src/agents/embedded-agent-subscribe.handlers.compaction.ts, src/agents/embedded-agent-subscribe.ts)
  • sandieman2: Authored a merged adjacent final-reply and routed-thread dedupe fix in the embedded runtime history. (role: adjacent delivery-dedupe contributor; confidence: low; commits: c67dc59b02b0; files: src/agents/embedded-agent-subscribe.ts)
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.

@jetd1
jetd1 force-pushed the fix/cross-run-messaging-dedupe-leak branch from 1916451 to 7fd065c Compare May 12, 2026 18:09
@jetd1
jetd1 requested a review from a team as a code owner May 12, 2026 18:09
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: discord Channel integration: discord channel: googlechat Channel integration: googlechat channel: imessage Channel integration: imessage channel: line Channel integration: line channel: matrix Channel integration: matrix channel: mattermost Channel integration: mattermost channel: msteams Channel integration: msteams channel: nextcloud-talk Channel integration: nextcloud-talk channel: nostr Channel integration: nostr channel: signal Channel integration: signal channel: slack Channel integration: slack channel: telegram Channel integration: telegram channel: tlon Channel integration: tlon channel: voice-call Channel integration: voice-call channel: whatsapp-web Channel integration: whatsapp-web channel: zalo Channel integration: zalo channel: zalouser Channel integration: zalouser extensions: copilot-proxy Extension: copilot-proxy extensions: diagnostics-otel Extension: diagnostics-otel extensions: llm-task Extension: llm-task extensions: lobster Extension: lobster extensions: memory-core Extension: memory-core extensions: memory-lancedb Extension: memory-lancedb labels May 12, 2026
@BradGroux BradGroux mentioned this pull request May 13, 2026
Draft
jet
fix: clear cross-run messaging-tool sent-text state after every compa…
a071872 
…ction

When a run completes and the session compacts, the arrays tracking
texts sent via messaging tools (message(action=send), sessions_send,
etc.) were only cleared on compaction retries (willRetry=true).
On normal compaction (willRetry=false), stale entries survived into
the next run, where isMessagingToolDuplicateNormalized() could
false-positive on the new run's final reply text — silently dropping
legitimate block replies.

Clear messagingToolSentTexts, messagingToolSentTextsNormalized,
messagingToolSentTargets, and messagingToolSentMediaUrls
unconditionally at every compaction_end so the next run always
starts with a clean slate.

Co-Authored-By: Cha <[email protected]> via OpenClaw
@MarkMa84 MarkMa84 mentioned this pull request May 19, 2026
Closed
@openclaw-barnacle

openclaw-barnacle Bot commented Jun 1, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Merged
@clawsweeper

clawsweeper Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@jetd1 thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jetd1