fix(embedded): scope messaging dedupe suppression to originating target#26889
fix(embedded): scope messaging dedupe suppression to originating target#26889kevinwildenradt wants to merge 62 commits intoopenclaw:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02cf8bd0ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1de25fb8ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bc990f7af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # src/agents/pi-embedded-subscribe.tools.extract.test.ts # src/auto-reply/reply/reply-payloads.ts
…-dedupe-target-scope
…-dedupe-target-scope # Conflicts: # src/agents/pi-embedded-runner/run/attempt.test.ts # src/security/dm-policy-shared.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9532e60eb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b7ee11a02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const target = parseTelegramTarget(params.targetKey); | ||
| const rawTargetThreadId = | ||
| typeof params.targetThreadId === "string" ? params.targetThreadId.trim() : undefined; | ||
| if (rawTargetThreadId && !/^\d+$/.test(rawTargetThreadId)) { |
There was a problem hiding this comment.
The new malformed-thread guard treats any targetThreadId that is not plain digits as off-target, but Telegram routing already accepts scoped thread IDs in the form <chatId>:<threadId> (parseTelegramThreadId in src/telegram/outbound-params.ts). Fresh evidence is that this commit introduced the strict ^\d+$ check, so valid scoped thread IDs are now rejected here. When a tool send uses that valid format, dedupe matching incorrectly returns false and the origin reply is emitted again as a duplicate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in c4268c5b4 (fix(reply): accept scoped telegram thread ids in dedupe).
targetsMatchForSuppression(...) now parses explicit Telegram threadId with the shared parseTelegramThreadId(...) helper, so valid scoped ids like <chatId>:<threadId> are treated as explicit topic ids (matching runtime routing semantics).
Behavior now:
threadId="-100123:77"matches topic77and allows suppression.threadId="-100123:88"remains off-target and does not suppress.- malformed explicit values still return off-target (no chat-level fallback).
Coverage added in src/auto-reply/reply/reply-payloads.test.ts for both scoped-match and scoped-cross-topic cases.
Validation:
pnpm vitest run --config vitest.config.ts src/auto-reply/reply/reply-payloads.test.tspnpm check
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4268c5b4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| const targetNormalizationProvider = isGenericMessageProvider ? provider : targetProvider; | ||
| const targetKey = normalizeTargetForProvider(targetNormalizationProvider, target.to); | ||
| const targetKey = normalizeTargetForProvider(targetProvider, target.to); |
There was a problem hiding this comment.
The new target-scoped dedupe check compares originatingTo against normalizeTargetForProvider(..., target.to) only, but target.to is captured from tool arguments before runtime target resolution. In valid flows where the message tool sends to the current chat via a resolvable alias (for example a directory name/handle), runMessageAction can resolve and rewrite the outbound target in resolveActionTarget(...), yet this suppression check still sees the unresolved alias and returns false. That causes same-chat sends to be treated as off-target and emits a duplicate block reply instead of suppressing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in d077b1b12 (fix(embedded): use resolved send target for dedupe scope).
I updated embedded subscribe tracking so target-scoped dedupe can use post-resolution routing data instead of only the pre-execution tool args:
- Added
extractMessagingToolSendFromResult(...)to extract send route metadata from tool result payloads (detailsor JSON text payload). - In
handleToolExecutionEnd(...), pending messaging target metadata is now enriched from resolved result metadata (fallback to post-call args), then committed for suppression matching.
This preserves scoped dedupe when a message send uses an alias/handle that resolves to the current chat at runtime.
Coverage added:
src/agents/pi-embedded-subscribe.handlers.tools.test.ts- updates pending messaging target from resolved tool result payload.
src/agents/pi-embedded-subscribe.tools.extract.test.ts- extracts normalized send target from JSON tool output/details.
Validation:
pnpm vitest run --config vitest.config.ts src/agents/pi-embedded-subscribe.tools.extract.test.ts src/agents/pi-embedded-subscribe.handlers.tools.test.ts src/auto-reply/reply/reply-payloads.test.tspnpm check
# Conflicts: # src/agents/pi-embedded-subscribe.handlers.messages.ts # src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.ts # src/agents/pi-embedded-subscribe.ts # src/auto-reply/reply/reply-payloads.test.ts # src/auto-reply/reply/reply-payloads.ts
|
Addressed current CI failure (secrets / pnpm-audit-prod) in aa51117.\n\nWhat changed:\n- Bumped extension peer floor for OpenClaw to patched range:\n - extensions/googlechat/package.json: openclaw >=2026.3.2\n - extensions/memory-core/package.json: openclaw >=2026.3.2\n- Refreshed pnpm lock entries so those importers resolve openclaw 2026.3.2 instead of 2026.3.1.\n\nValidation run locally:\n- pnpm audit --prod --audit-level=high (passes; only 1 low remains)\n- pnpm check\n\nNo review threads were resolved in this update. |
openclaw-barnacle
bot
added
channel: googlechat
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa5111746b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (origin.messageThreadId && !targetThreadId) { | ||
| return true; |
There was a problem hiding this comment.
targetsMatchForSuppression currently treats targetThreadId with a falsy check, so an explicit threadId that parses to 0 is handled as “no thread” and falls into the auto-threading match path. In Telegram runs originating from a topic, this can suppress the origin reply even when the tool send did not stay in that topic (for example, an explicit threadId: "0" that downstream send logic treats as invalid/off-topic). This regresses target-scoped dedupe by turning an explicit thread choice into chat-level fallback matching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in cf1013727 (fix(reply): treat explicit telegram threadId 0 as explicit target).\n\ntargetsMatchForSuppression(...) now treats an explicit threadId as explicit even when it parses to 0, so topic-origin flows no longer fall through to chat-level auto-thread fallback for threadId: "0".\n\nBehavior now:\n- topic origin + explicit threadId: "0" => off-target (no suppression)\n- topic origin + missing threadId => existing auto-thread fallback unchanged\n\nAlso fixed current CI failure in src/plugins/commands.test.ts by aligning expected normalized command metadata (acceptsArgs: false).\n\nValidation run locally:\n- pnpm exec vitest run --config vitest.config.ts src/plugins/commands.test.ts src/auto-reply/reply/reply-payloads.test.ts
|
Pushed structural dedupe cleanup in commit 4feb765 to reduce edge-case regressions across follow-up reviews. What changed:
Validation:
Review threads remain unresolved as requested. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4feb7655f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return true; | ||
| } | ||
|
|
||
| const sentTargets = collectSentTargets(params.sentRecords); |
There was a problem hiding this comment.
This function checks hasDuplicate for the current normalizedText, but then evaluates routing scope against collectSentTargets(params.sentRecords) from the entire run. In mixed runs, a different message sent to the origin target can make shouldSuppressMessagingToolReplies(...) return true and suppress a reply whose duplicated text was actually only sent off-target. To avoid cross-target suppression in this case, target matching should only consider records whose textNormalized matches the candidate reply text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 677134971 (fix(embedded): scope dedupe target checks to matching text records).
I updated shouldSuppressMessagingToolBlockReply(...) to scope routing checks to matching-text records only:
- compute
matchingRecordsfor the candidatenormalizedText - evaluate
shouldSuppressMessagingToolReplies(...)using targets extracted from those matching records only
This prevents in-scope sends of different text from suppressing a duplicated off-target text.
Coverage added in src/agents/pi-embedded-subscribe.dedupe.test.ts:
scopes routing checks to records for the duplicated text
Validation:
pnpm exec vitest run --config vitest.config.ts src/agents/pi-embedded-subscribe.dedupe.test.ts src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.tspnpm check
| // Keep fallback suppression only for matching text from non-explicit targets. | ||
| return params.sentRecords.some( | ||
| (record) => | ||
| record.textNormalized === params.normalizedText && record.targetSource !== "explicit", |
There was a problem hiding this comment.
The final fallback suppresses any matching record where targetSource !== "explicit", but "inferred" records can still have a concrete resolved target (for example when handleToolExecutionEnd derives one from afterToolCallArgs/result without a pending explicit target). If that inferred target is off-target, routing checks correctly return false and this fallback then suppresses anyway, reintroducing false suppression. The fallback should be restricted to records that truly lack target metadata rather than all non-explicit sources.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 677134971 as part of the same dedupe-gate update.
I narrowed the fallback path so it only suppresses matching-text records that truly lack target metadata (!record.target).
Previously fallback used targetSource !== "explicit", which could incorrectly include inferred records that already had resolved off-target metadata. With the new logic:
- inferred records with concrete off-target target metadata no longer fallback-suppress
- targetless matching records still preserve legacy inferred-target fallback behavior
Coverage added in src/agents/pi-embedded-subscribe.dedupe.test.ts:
does not fallback-suppress inferred records when they include off-target metadatakeeps fallback suppression for matching records without target metadata
Validation:
- targeted vitest command above +
pnpm checkboth pass locally.
There was a problem hiding this comment.
Fixed in 677134971 as part of the same dedupe-gate update.
I narrowed the fallback path so it only suppresses matching-text records that truly lack target metadata (!record.target).
Previously fallback used targetSource !== "explicit", which could incorrectly include inferred records that already had resolved off-target metadata. With the new logic:
- inferred records with concrete off-target target metadata no longer fallback-suppress
- targetless matching records still preserve legacy inferred-target fallback behavior
Coverage added in src/agents/pi-embedded-subscribe.dedupe.test.ts:
does not fallback-suppress inferred records when they include off-target metadatakeeps fallback suppression for matching records without target metadata
Validation:
- targeted vitest command above +
pnpm checkboth pass locally.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 677134971b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isToolError && committedTarget) { | ||
| ctx.state.messagingToolSentTargets.push(committedTarget); | ||
| ctx.trimMessagingToolSent(); |
There was a problem hiding this comment.
This branch records committedTarget for any successful messaging tool call, even when the tool action was not a send. For message actions like poll, the tool result payload still includes channel/to (see src/infra/outbound/message.ts), so extractMessagingToolSendFromResult(...) infers a target and this code appends it to messagingToolSentTargets. Downstream suppression (shouldSuppressMessagingToolReplies) can then treat the run as already replied and drop the final assistant reply entirely, causing silent post-action responses for non-send actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in f38f5fe86 (fix(embedded): ignore non-send message actions in dedupe target tracking).
I updated handleToolExecutionEnd(...) so target extraction/commit is gated by isMessagingSendCall, not just isMessagingTool(...):
- derive
isMessagingSendCallfromisMessagingToolSendAction(...)using both start and adjusted args - only extract/commit
committedTargetwhen that call is a send action
This prevents non-send message actions (for example poll) from contributing dedupe targets via result payloads.
Regression coverage added:
src/agents/pi-embedded-subscribe.handlers.tools.test.tsdoes not commit targets from non-send message actions
Validation:
pnpm exec vitest run --config vitest.config.ts src/agents/pi-embedded-subscribe.handlers.tools.test.ts src/agents/pi-embedded-subscribe.dedupe.test.ts src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.tspnpm check
|
Superseded by #33786. The synthesis PR integrates the relevant shared dedupe/routing semantics and tests while avoiding unrelated churn. |
|
Thanks again for this work. This PR was triaged and merged using an AI-assisted review/synthesis workflow. This landed indirectly via the synthesized PR #33786, and your contribution is credited in the changelog and as a co-author on the merge commit. Closing this PR as superseded by #33786 (reason: superseded). If anything here looks incorrect or incomplete, reply to reopen and we can reassess. |
|
Correction for accuracy: the synthesized work is credited in the changelog, but the squash merge commit for #33786 does not include explicit |
2 participants
Summary
Fix false duplicate suppression in embedded subscribe when the message tool sends to a different destination than the current conversation.
Problem
In multi-chat flows (notably BlueBubbles), a run can send text via the
messagetool to chat B while still answering chat A.If the assistant answer matches the tool-sent text, current dedupe logic may suppress the reply in chat A, making the agent appear stalled.
Root Cause
Duplicate suppression for block replies (
text_endandmessage_endpaths) used normalized sent text without checking whether the message tool send target matched the current run’s originating target.Changes
messageProvideroriginatingToaccountIdshouldSuppressMessagingToolReplies(...)so suppression only applies in-scope.message_endshould not be suppressedtext_endshould not be suppressedRepro (before)
Xto chat B.X.Behavior After Fix
Suppression only occurs when the message tool target matches the run’s originating target. Cross-target sends no longer silence the origin chat reply.
Validation
src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.suppresses-message-end-block-replies-message-tool.test.tspnpm/corepackwas unavailable; please rely on CI and/or run the targeted command locally.Greptile Summary
Scopes messaging tool duplicate suppression to only apply when the message tool target matches the originating conversation target. Previously, if an agent sent a message to chat B while answering chat A, and the text matched, the reply in chat A would incorrectly be suppressed.
Key changes:
messageProvider,originatingTo,accountId) tosubscribeEmbeddedPiSessionparametersshouldSuppressMessagingToolReplieshelper that checks if any sent target matches the originating targetemitBlockChunk(text_end path) andhandleMessageEnd(message_end path) to only suppress when targets matchConfidence Score: 5/5
Last reviewed commit: 02cf8bd
(2/5) Greptile learns from your feedback when you react with thumbs up/down!