fix(outbound): check delivery identity for media results before recording success#93401
Closed
xzh-icenter wants to merge 2 commits into
Closed
fix(outbound): check delivery identity for media results before recording success#93401xzh-icenter wants to merge 2 commits into
xzh-icenter wants to merge 2 commits into
Conversation
…n show' so rebind detects changed roots When a managed QMD collection's root path changes (e.g. workspace repoint), ensureCollections() should detect the change and rebind (remove + re-add) the collection. The rebind decision in shouldRebindCollection() compares the listed path against the desired collection path, but listCollectionsBestEffort() only runs 'qmd collection list --json' which omits the filesystem path. With listed.path always undefined, shouldRebindCollection() takes its defensive incomplete-metadata branch and returns false — collections never rebind. Add enrichCollectionPaths() which runs 'qmd collection show <name>' for each listed managed collection whose path is still missing, extracting the Path line to populate the path field so shouldRebindCollection() can detect a changed root and trigger a rebind. Closes openclaw#91251
…ding success The sendPayload branch checks hasDeliveryResultIdentity before counting a delivery result as sent, but the media branch was missing this check. When a channel adapter returns a media result with no messageId or other identity fields (e.g. QQBot rejecting a TTS temp file outside its media roots), the delivery was incorrectly recorded as successful. This caused cron auto-TTS deliveries to QQBot to be reported as delivered when the message was actually lost, since the generated audio file was outside QQBot's allowed media roots and the adapter returned an error result with no delivery identity. Add the same hasDeliveryResultIdentity check to the media delivery loop so that no-identity media results are properly skipped, consistent with the sendPayload branch behavior. Fixes openclaw#92816
openclaw-barnacle
Bot
added
extensions: memory-core
Contributor
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the only useful remaining outbound accounting change is already covered by the broader, clean, proof-positive PR at #79811, while this branch is dirty and also carries unrelated already-main QMD work. Canonical path: Close this PR and keep review focused on #79811; if maintainers reject that broader path, a fresh narrow branch should be opened from current main. So I’m closing this here and keeping the remaining discussion on #79811. Review detailsBest possible solution: Close this PR and keep review focused on #79811; if maintainers reject that broader path, a fresh narrow branch should be opened from current main. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: current main can append a no-identity media result and classify a non-empty delivered slice as sent. I did not run a live QQBot cron reproduction for this PR. Is this the best way to solve the issue? No, this is not the best landing path. The media-only patch is plausible but incomplete, and #79811 is the cleaner canonical fix because it applies the identity-required contract across all shared empty-receipt paths with proof and tests. Security review: Security review cleared: No concrete security or supply-chain issue was found; the diff changes TypeScript delivery/memory logic only and does not alter dependencies, workflows, secrets, or package execution surfaces. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against dc573a38dc9c. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
Contributor
|
ClawSweeper applied the proposed close for this PR.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
sendPayloadbranch checkshasDeliveryResultIdentitybefore counting a delivery result as sent, but the media branch was missing this check. When a channel adapter returns a media result with no messageId or other identity fields (e.g. QQBot rejecting a TTS temp file outside its media roots), the delivery was incorrectly recorded as successful.This caused cron auto-TTS deliveries to QQBot to be reported as delivered when the message was actually lost, since the generated audio file was outside QQBot's allowed media roots and the adapter returned an error result with no delivery identity.
Real behavior proof
From source inspection of current main:
Cron applies auto-TTS before delivery:
src/cron/isolated-agent/delivery-dispatch.ts:894transforms payloads throughmaybeApplyTtsToCronPayloadsbeforesendDurableMessageBatch, replacing text with generated audio whenmessages.tts.auto="always".TTS creates local temp audio:
packages/speech-core/src/tts.ts:2090writes synthesized audio under the preferred temp root (e.g./tmp/openclaw/tts-*/voice-*.mp3).QQBot rejects temp files outside media roots:
extensions/qqbot/src/engine/messaging/outbound-media-send.ts:132validates local media paths against allowed media roots. Generated TTS temp files outside those roots produce an error result with no delivery identity.QQBot returns no-identity result:
extensions/qqbot/src/channel.ts:124converts engine results withmessageId: result.messageId ?? "", so an error still returns an object with no delivery identity.Media branch lacks identity check:
src/infra/outbound/deliver.ts:1894pushes every media result and counts it as sent based on a non-empty slice. Unlike thesendPayloadbranch at line 1730, it does not checkhasDeliveryResultIdentitybefore classifying success.Fix
Add
hasDeliveryResultIdentitycheck to the media delivery loop indeliver.ts, consistent with thesendPayloadbranch behavior. No-identity media results are now properly skipped instead of being counted as sent.Testing
The existing test in
src/infra/outbound/deliver.test.tscovers the media delivery path with valid results. This fix adds the missing identity check that prevents no-identity results from being counted as sent.Related