Skip to content

fix: deliver compaction.notifyUser notices via routeReply when no active turn#88979

Open
yichu10c wants to merge 1 commit into
openclaw:mainfrom
yichu10c:fix/88933-compaction-notifyuser-fallback
Open

fix: deliver compaction.notifyUser notices via routeReply when no active turn#88979
yichu10c wants to merge 1 commit into
openclaw:mainfrom
yichu10c:fix/88933-compaction-notifyuser-fallback

Conversation

@yichu10c

@yichu10c yichu10c commented Jun 1, 2026

Copy link
Copy Markdown

Fixes #88933

Summary

When agents.defaults.compaction.notifyUser: true is configured but no active streaming turn callback (onBlockReply) is available, sendCompactionNotice now falls back to routing the notice through the canonical session outbound path (routeReply) instead of silently returning early.

This fixes the issue where async/preflight compactions (triggered outside of active inbound-driven streaming reply turns — e.g. proactive maxHistoryShare compactions, subagent-completion auto-announce compactions) produce no user-visible notice even when notifyUser: true is configured.

Root cause

sendCompactionNotice was guarded by an early return when params.opts?.onBlockReply was absent. All three call sites (start/end/incomplete phases) go through this guard, so any compaction without an active streaming turn silently skips notification.

Fix

The fix adds a fallback delivery path:

  1. Primary path (unchanged): deliver via onBlockReply when available
  2. Fallback path (new): when onBlockReply is absent but notifyUser is enabled, route the notice through routeReply using the session's channel/surface info

The fallback only activates when:

  • notifyUser is true in runtime config
  • The session has a routable channel (Surface ?? Provider)
  • The session has a valid to address (OriginatingTo ?? To)

Errors in both paths are caught and logged as non-fatal (fire-and-forget semantics preserved).

Tests

All 152 tests in src/auto-reply/reply/agent-runner-execution.test.ts pass, including the 9 compaction-related tests:

node scripts/run-vitest.mjs src/auto-reply/reply/agent-runner-execution.test.ts
✓ auto-reply  src/auto-reply/reply/agent-runner-execution.test.ts (152 tests) 1715ms
Test Files  1 passed (1)
     Tests  152 passed (152)

Review notes

  • Clawsweeper previously marked this issue clawsweeper:fix-shape-clear and clawsweeper:queueable-fix — the fix follows the identified canonical delivery seam (routeReply → sendDurableMessageBatch).
  • The fix reuses existing ReplyDispatchKind.block for compaction notices, consistent with their transient/status semantics.
  • A subsequent PR could add a unit test specifically covering the no-onBlockReply fallback path for completeness.

@yichu10c
fix: deliver compaction.notifyUser notices via routeReply fallback
1e07aa0 
When compaction.notifyUser is enabled but no active streaming turn callback
(onBlockReply) is available, sendCompactionNotice now falls back to routing
the notice through the canonical session outbound path (routeReply) instead
of silently returning.

This fixes the issue where async/preflight compactions (triggered outside
of active inbound-driven streaming reply turns) produce no user-visible
notice even when notifyUser: true is configured.

Fixes openclaw#88933
@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 1, 2026
@clawsweeper

clawsweeper Bot commented Jun 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 1, 2026, 3:21 AM ET / 07:21 UTC.

Summary
The PR routes compaction.notifyUser notices through routeReply when no active onBlockReply callback is available.

PR surface: Source +38. Total +38 across 1 file.

Reproducibility: yes. source-level reproduction is high confidence: current main returns from sendCompactionNotice when opts.onBlockReply is absent even with notifyUser: true, while docs say notices should be sent. I did not run a live channel repro in this read-only pass.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • Use the canonical effective route, including OriginatingChannel, for fallback delivery.
  • [P1] Add a focused no-onBlockReply regression test for notifyUser: true.
  • Post redacted real behavior proof from an affected channel/session after the patch.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body only reports a unit Vitest run; it does not show after-fix delivery in a real channel/session, so contributor proof is still needed with private details redacted. 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

  • [P2] Merging as-is can still drop or misroute fallback compaction notices when OriginatingChannel differs from Surface/Provider, especially for route-only or internal-source delivery contexts.
  • [P1] The PR has only unit-test output in the body; no real channel/session proof shows the after-fix notice being delivered.

Maintainer options:

  1. Fix fallback route selection (recommended)
    Resolve the canonical effective reply route before calling routeReply, including OriginatingChannel and inherited external delivery contexts.
  2. Accept a narrower fallback
    Maintainers could intentionally limit this PR to simple Surface/Provider sessions, but that leaves the reported route-only async class partially uncovered.
  3. Pause for contributor proof
    Keep the PR open until the author posts redacted real channel proof and updates the branch with fallback-route coverage.

Next step before merge

  • [P1] The branch needs contributor-visible fixes and real behavior proof; ClawSweeper should not queue an automated repair while the external proof gate remains unmet.

Security
Cleared: The diff touches runtime message delivery only and does not add dependencies, workflow changes, secret handling, or new code execution surfaces.

Review findings

  • [P2] Use the canonical origin route for fallback notices — src/auto-reply/reply/agent-runner-execution.ts:1598
  • [P3] Inspect routeReply failures before swallowing the notice — src/auto-reply/reply/agent-runner-execution.ts:1609
Review details

Best possible solution:

Use the same effective origin/session delivery route as final replies, handle routeReply { ok: false } results, add focused no-onBlockReply coverage, and require redacted real channel proof before merge.

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

Yes, source-level reproduction is high confidence: current main returns from sendCompactionNotice when opts.onBlockReply is absent even with notifyUser: true, while docs say notices should be sent. I did not run a live channel repro in this read-only pass.

Is this the best way to solve the issue?

No. routeReply is the right seam, but this implementation should use the canonical effective route and inspect non-throwing routeReply failures before it is the best fix.

Full review comments:

  • [P2] Use the canonical origin route for fallback notices — src/auto-reply/reply/agent-runner-execution.ts:1598
    This fallback ignores OriginatingChannel and derives the channel from Surface ?? Provider. The rest of reply routing treats OriginatingChannel/OriginatingTo as the delivery contract, with special handling for internal/system contexts; when Surface is internal or stale while the origin is external, this branch returns early or routes to the wrong provider, so the async notifyUser notice can still disappear. Resolve the same effective route used for session/final delivery before calling routeReply.
    Confidence: 0.88
  • [P3] Inspect routeReply failures before swallowing the notice — src/auto-reply/reply/agent-runner-execution.ts:1609
    routeReply returns { ok: false, error } for send failures instead of throwing, but this new fallback only logs thrown exceptions. A failed fallback delivery will therefore be silent, which preserves the observability gap this path is trying to close; check the result and log result.error when ok is false.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 5e09113edec4.

Label changes

Label changes:

  • add P2: This is a focused user-visible message-delivery bug fix with limited blast radius, matching the linked issue's normal-priority impact.
  • add merge-risk: 🚨 message-delivery: The new fallback delivery path can still drop or misroute notices if it uses Surface/Provider instead of the canonical origin route.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add 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 only reports a unit Vitest run; it does not show after-fix delivery in a real channel/session, so contributor proof is still needed with private details redacted. 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.

Label justifications:

  • P2: This is a focused user-visible message-delivery bug fix with limited blast radius, matching the linked issue's normal-priority impact.
  • merge-risk: 🚨 message-delivery: The new fallback delivery path can still drop or misroute notices if it uses Surface/Provider instead of the canonical origin route.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • 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 only reports a unit Vitest run; it does not show after-fix delivery in a real channel/session, so contributor proof is still needed with private details redacted. 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 +38. Total +38 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 45 7 +38
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 45 7 +38

What I checked:

Likely related people:

  • vincentkoc: Current blame for sendCompactionNotice, routeReply, and resolveEffectiveReplyRoute points to the recent refactor commit that carried these paths on main. (role: recent area contributor; confidence: medium; commits: e843a3612b57; files: src/auto-reply/reply/agent-runner-execution.ts, src/auto-reply/reply/route-reply.ts, src/auto-reply/reply/effective-reply-route.ts)
  • steipete: The verbose auto-compaction notice and compaction count behavior appears to date to feat: track compaction count + verbose notice. (role: feature introducer; confidence: medium; commits: b30bae89edd8; files: src/auto-reply/reply/followup-runner.ts, src/auto-reply/reply/agent-runner.ts)
  • jalehman: The originating-channel reply routing contract and route-reply.ts were introduced by the routing feature commit authored by Josh Lehman. (role: origin-routing feature owner; confidence: high; commits: 9d50ebad7d33; files: src/auto-reply/reply/route-reply.ts, src/auto-reply/templating.ts, src/auto-reply/reply/followup-runner.ts)
  • uf-hy: Recent compaction accounting work changed agent-runner-execution.ts, followup-runner.ts, and adjacent tests for auto-compaction behavior. (role: recent compaction contributor; confidence: medium; commits: 3928b4872ab1; files: src/auto-reply/reply/agent-runner-execution.ts, src/auto-reply/reply/followup-runner.ts, src/auto-reply/reply/followup-runner.test.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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 1, 2026
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

merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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.

Bug: compaction.notifyUser notices gated behind onBlockReply — async compactions silently notify nothing

1 participant

@yichu10c