fix(reply): preserve pending thread evidence when reconciling partial send results by yetval · Pull Request #93291 · openclaw/openclaw

yetval · 2026-06-15T12:50:26Z

Summary

In a Mattermost thread, when the agent posts into the current channel via the message tool (implicit threading, no explicit replyToId) and then emits its final composed reply, the user receives the reply twice in the thread. This hits both the native embedded-agent harness and the Codex harness.

The post-send reconciler extractMessagingToolSendResult re-derives the thread fields from the provider result and erases the pending send's thread evidence. This change makes a partial provider result keep the pending thread evidence it never speaks to, so same-thread reply suppression works again.

Root cause

extractMessagingToolSendResult (src/agents/embedded-agent-subscribe.tools.ts) reconciles the pending send evidence with the provider's extractToolSendResult, but unconditionally re-derives the thread fields from the provider result:

// before
threadId: normalizeOptionalString(extracted.threadId),
threadImplicit: extracted.threadImplicit === true ? true : undefined,
threadSuppressed: extracted.threadSuppressed === true ? true : undefined,

Mattermost is the only production provider that implements extractToolSendResult, and extractMattermostToolSendResult (extensions/mattermost/src/channel.ts) returns only { to, threadId? }, with threadId present only when an explicit replyToId was passed. So for an implicitly threaded send the provider result is just { to }, and the three lines above overwrite the correct pending threadId: "root-1" + threadImplicit: true with undefined.

Mattermost's outbound adapter has no targetsMatchForReplySuppression matcher, so once the thread id is gone the dedupe guard in src/auto-reply/reply/reply-payloads-dedupe.ts bails (originRoute.threadId != null but targetRoute.threadId is now null) and returns no match. The final reply is therefore not suppressed and is delivered on top of the tool send.

This was introduced by c67dc59 (#90943): the call site in embedded-agent-subscribe.handlers.tools.ts changed from pushing ...pendingTarget (full evidence) to ...confirmedTarget (reconciled). The same reconciler is also invoked from the Codex app-server (extensions/codex/src/app-server/dynamic-tools.ts), so the defect reaches both harnesses.

Fix

A partial provider result must not erase pending evidence it never reports:

const extractedThreadId = normalizeOptionalString(extracted.threadId);
const providerReportedThread = extractedThreadId != null;
// ...
threadId: extractedThreadId ?? pending.threadId,
threadImplicit:
  extracted.threadImplicit === true ||
  (!providerReportedThread && pending.threadImplicit === true)
    ? true
    : undefined,
threadSuppressed:
  extracted.threadSuppressed === true ||
  (!providerReportedThread && pending.threadSuppressed === true)
    ? true
    : undefined,

When the provider reports an explicit thread it still wins (and clears the implicit flag, since an explicit thread is not implicit); when it does not, the pending threadId/threadImplicit/threadSuppressed survive.

Why not one-sided

The fix is in the shared reconciler extractMessagingToolSendResult, the single chokepoint used by both call sites (the native embedded-agent-subscribe.handlers.tools.ts and the Codex extensions/codex/src/app-server/dynamic-tools.ts), so both harnesses are covered by one change.
It is provider-agnostic: any provider whose extractToolSendResult returns a partial result (Mattermost today) keeps its pending thread evidence, while a provider that does report a thread still has it honored.
The explicit-thread path is preserved: an explicitly reported threadId overrides pending evidence and clears threadImplicit, so genuine thread changes are not masked.

Verification

node scripts/run-vitest.mjs run src/agents/embedded-agent-subscribe.tools.reconcile-thread.test.ts src/agents/embedded-agent-subscribe.tools.extract.test.ts src/auto-reply/reply/reply-payloads.test.ts extensions/codex/src/app-server/dynamic-tools.test.ts -> 100 passed (adds 2 regression tests: implicit evidence preserved through a partial result and then suppressed by dedupe; explicit provider thread still overrides). The new test fails on pristine main (expected undefined to be true) and passes with this patch.
oxfmt --check on the changed files -> clean.
oxlint --type-aware --tsconfig config/tsconfig/oxlint.core.json on the changed files -> 0 findings.
tsgo:core + tsgo:core:test -> clean.

Real behavior proof

Behavior addressed: an implicitly threaded Mattermost message-tool send no longer loses its thread evidence during reconciliation, so the agent's final reply is suppressed by same-thread dedupe instead of being delivered a second time in the thread.

Real environment tested: a throwaway harness (not committed) drove the real production functions from this branch end to end with the real mattermostPlugin registered: extractMessagingToolSend -> extractMessagingToolSendResult -> getMatchingMessagingToolReplyTargets, run against the pristine build and the patched build.

Exact steps or command run after this patch: register the Mattermost plugin, build the pending send for an implicit reply into thread root-1 on channel:abc (no explicit replyToId), reconcile it against the real Mattermost-shaped result { details: { toolSend: { to: "channel:abc" } } }, then ask the real dedupe whether the final reply into root-1 is suppressed.

Evidence after fix:

# BEFORE (pristine main 9ba6ed1d5c)
  pending  threadImplicit = true  threadId = "root-1"
  confirmed threadImplicit = undefined  threadId = undefined
  same-thread reply suppression matches = 0
  result: final reply DELIVERED on top of tool send (DUPLICATE in thread)

# AFTER (this patch, identical inputs)
  pending  threadImplicit = true  threadId = "root-1"
  confirmed threadImplicit = true  threadId = "root-1"
  same-thread reply suppression matches = 1
  result: final reply SUPPRESSED (no duplicate)

Observed result after fix: the same implicit-thread send whose reconciled target previously dropped to threadImplicit=undefined/threadId=undefined (yielding 0 suppression matches, a duplicate reply) now keeps threadImplicit=true/threadId="root-1" and produces 1 suppression match, so the final reply is suppressed.

What was not tested: a live end-to-end run against a real Mattermost server; this change is at the reconciler/dedupe layer and is covered above by driving the real runtime functions plus the colocated regression coverage on both the native and Codex test suites.

Regression introduced by c67dc59 (#90943).

clawsweeper · 2026-06-15T12:52:17Z

Codex review: needs maintainer review before merge. Reviewed June 15, 2026, 3:35 PM ET / 19:35 UTC.

Summary
The PR updates extractMessagingToolSendResult to preserve pending thread evidence when provider send results omit thread fields, and adds a core-local regression test for partial-result reply dedupe.

PR surface: Source +10, Tests +97. Total +107 across 2 files.

Reproducibility: yes. Source inspection shows current main drops pending thread fields when a provider result omits them, Mattermost can omit threadId for implicit sends, and dedupe then lacks the target thread evidence needed to suppress the final reply.

Review metrics: none identified.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Mantis proof suggestion
A real Mattermost thread proof would materially improve confidence for the visible duplicate-reply behavior, even though unit-level review is clean. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify an implicit Mattermost thread message-tool send suppresses the final reply instead of posting a duplicate.

Risk before merge

[P1] The reconciler controls whether final replies are suppressed or delivered after message-tool sends; a wrong merge could duplicate, drop, or wrongly suppress threaded channel messages.
[P1] No live Mattermost server run was supplied, so maintainers may still request transport-level proof even though the PR has focused real-function proof and green CI.

Maintainer options:

Accept the current proof (recommended)
Maintainers can land after ordinary review because the fix is localized to the shared reconciler, has focused regression coverage, and includes structured before/after real-function proof.
Request live Mattermost confirmation
If transport-level confidence is required, ask for a redacted Mattermost thread run showing the message-tool send suppressing the final reply.

Next step before merge

[P2] No repair lane is needed; maintainers should decide whether the supplied function-level proof is enough or request live Mattermost proof before merge.

Security
Cleared: The diff changes TypeScript runtime logic and tests only; it does not touch workflows, dependencies, lockfiles, package scripts, credentials, or artifact download paths.

Review details

Best possible solution:

Land the shared reconciler fix with the core-local regression test; live Mattermost proof is an optional maintainer confidence check, not a required code repair from this review.

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

Yes. Source inspection shows current main drops pending thread fields when a provider result omits them, Mattermost can omit threadId for implicit sends, and dedupe then lacks the target thread evidence needed to suppress the final reply.

Is this the best way to solve the issue?

Yes. The shared reconciler is the narrowest maintainable fix boundary because native, CLI, and Codex callers use it; a Mattermost-only patch would leave the generic partial-result contract broken.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

P1: The PR fixes a user-visible duplicate final-reply path in threaded Mattermost agent workflows, which is an urgent channel-delivery regression before release.
merge-risk: 🚨 message-delivery: The diff changes preserved route/thread evidence that decides whether final replies are suppressed or delivered after message-tool sends.
rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body supplies structured before/after live output from a throwaway harness driving the real reconciler, dedupe path, and Mattermost plugin result shape; no contributor proof action is needed.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies structured before/after live output from a throwaway harness driving the real reconciler, dedupe path, and Mattermost plugin result shape; no contributor proof action is needed.

Evidence reviewed

PR surface:

Source +10, Tests +97. Total +107 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	13	3	+10
Tests	1	97	0	+97
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	110	3	+107

What I checked:

PR-head fix boundary: PR head keeps pending.threadId, threadImplicit, and threadSuppressed when the provider result reports no thread, while still letting a provider-reported thread override pending evidence. (src/agents/embedded-agent-subscribe.tools.ts:848, ca07847b82ae)
Current-main bug path: Current main overwrites threadId, threadImplicit, and threadSuppressed directly from the extracted provider result, so omitted provider fields erase pending implicit-thread evidence. (src/agents/embedded-agent-subscribe.tools.ts:953, b3128ba93def)
Mattermost partial result shape: Mattermost's successful send-result extractor returns to and only includes threadId when the provider result has one, matching the partial-result case this PR repairs. (extensions/mattermost/src/channel.ts:213, b3128ba93def)
Reply dedupe needs target thread evidence: Reply suppression builds the target route from target.threadId or implicit origin thread evidence and refuses channel-only fallback when thread evidence exists without a plugin matcher. (src/auto-reply/reply/reply-payloads-dedupe.ts:309, b3128ba93def)
Native caller uses reconciled target: The embedded-agent tool-end path pushes extractMessagingToolSendResult output into messagingToolSentTargets, so the reconciler decides native reply-suppression evidence. (src/agents/embedded-agent-subscribe.handlers.tools.ts:1227, b3128ba93def)
Codex caller uses same reconciler: The Codex dynamic-tool bridge calls extractMessagingToolSendResult before collecting message-tool telemetry, so the shared fix covers the Codex app-server path. (extensions/codex/src/app-server/dynamic-tools.ts:343, b3128ba93def)

Likely related people:

sandieman2: The related merged route/thread PR added the shared reconciler and route/thread dedupe series that this PR repairs. (role: introduced behavior in merged PR; confidence: high; commits: f99d95539a4c, c67dc59b02b0; files: src/agents/embedded-agent-subscribe.tools.ts, src/auto-reply/reply/reply-payloads-dedupe.ts)
steipete: The related route/thread PR was merged by this person, and its history includes adjacent Mattermost, Codex, and reply-route work. (role: merger and recent adjacent contributor; confidence: high; commits: 49d02f14565b, cf26fc62390b, c67dc59b02b0; files: extensions/mattermost/src/channel.ts, extensions/codex/src/app-server/dynamic-tools.ts, src/agents/embedded-agent-subscribe.handlers.tools.ts)
vincentkoc: Current main added CLI message-delivery evidence that also calls the shared reconciler, making this person a useful routing candidate for the new adjacent caller surface. (role: recent adjacent caller contributor; confidence: medium; commits: 0ea08076c3b5; files: src/agents/cli-runner/execute.ts, src/agents/embedded-agent-subscribe.tools.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.

yetval · 2026-06-15T13:13:35Z

@clawsweeper re-review please.

Rank-up since the last push:

Removed the core test's deep import of the bundled Mattermost runtime (extensions/mattermost/src/channel). That import tripped the core/extension package boundary and was the only cause of the red CI: boundary-invariants "keeps core tests off bundled extension deep imports", extension-test-boundary, and check-tsgo-core-boundary (which was pulling extensions/mattermost/src/{secret-input,runtime}.ts transitively).
Replaced it with a core-local channel test plugin that reproduces the same contract: an implicit-threading extractToolSend, a partial extractToolSendResult that reports only { to, threadId? }, and no targetsMatchForReplySuppression matcher. The test now exercises the generic reconciler contract with zero extension dependency.

Verification after the change:

Boundary checks that failed on the prior head now pass locally: boundary-invariants 14/14, check-tsgo-core-boundary exit 0, full-core-support-boundary config 97/97.
Regression guard intact: the test fails on pristine main (expected undefined to be true) and passes with the fix (2/2).
oxfmt clean, oxlint type-aware 0 findings, tsgo:core:test clean.

The production fix in extractMessagingToolSendResult is unchanged from the prior review.

clawsweeper · 2026-06-15T13:13:38Z

🦞🧹
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:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/27549391912
Updated: 2026-06-15T13:33:39.608Z

… send results extractMessagingToolSendResult re-derived threadId/threadImplicit/threadSuppressed straight from the provider result. Mattermost is the only production provider that implements extractToolSendResult, and for an implicitly threaded send it reports only { to }, so the reconciler overwrote the correct pending thread evidence with undefined. That defeated same-thread reply suppression in reply-payloads dedupe and delivered the agent's final reply twice in the thread, on both the native and Codex harnesses. A partial provider result now keeps the pending thread evidence it does not speak to: a provider-reported threadId still wins (and clears the implicit flag), but an absent one no longer erases the pending threadId/threadImplicit/threadSuppressed. Regression introduced by c67dc59 (openclaw#90943).

…ttermost import The reconcile-thread regression test deep-imported extensions/mattermost from a core test, which trips the core/extension package boundary (boundary-invariants "keeps core tests off bundled extension deep imports", extension-test-boundary, and check-tsgo-core-boundary pulling extensions/mattermost transitively). Replace it with a core-local channel test plugin that reproduces the same contract: an implicit-threading extractToolSend, a partial extractToolSendResult that reports only { to, threadId? }, and no targetsMatchForReplySuppression matcher. The test now exercises the generic reconciler contract with no extension dependency. It still fails on pristine main and passes with the fix.

vincentkoc · 2026-06-16T07:09:56Z

Land-ready verification:

Exact head: 3099983befea92a969b9f2ae7a83723ebf45b3a5.
Focused reconciliation/dedupe proof: 189 tests passed.
Targeted oxfmt and oxlint passed.
AWS Crabbox check:changed passed: lease cbx_0060adf347f2, run run_b0e5c0bab042.
Fresh Codex autoreview reported no actionable findings (patch is correct, confidence 0.94).
Exact-head CI completed. The remaining failures are unchanged failures on current main 63825369a24db68fb553b3df6320be8f318a6937: extensions/codex/src/app-server/bounded-turn.ts TS2322 and the three src/skills/runtime/session-snapshot.test.ts assertions. Main run: https://github.com/openclaw/openclaw/actions/runs/27599570896

The patch atomically selects provider-reported or pending thread evidence, preserving implicit/suppressed thread facts for partial provider results while allowing explicit provider thread results to win.

Known gap: no live Mattermost server run; the real production reconciler and reply-suppression path are covered directly.

… send results (openclaw#93291) * fix(reply): preserve pending thread evidence when reconciling partial send results extractMessagingToolSendResult re-derived threadId/threadImplicit/threadSuppressed straight from the provider result. Mattermost is the only production provider that implements extractToolSendResult, and for an implicitly threaded send it reports only { to }, so the reconciler overwrote the correct pending thread evidence with undefined. That defeated same-thread reply suppression in reply-payloads dedupe and delivered the agent's final reply twice in the thread, on both the native and Codex harnesses. A partial provider result now keeps the pending thread evidence it does not speak to: a provider-reported threadId still wins (and clears the implicit flag), but an absent one no longer erases the pending threadId/threadImplicit/threadSuppressed. Regression introduced by 47e4bdb (openclaw#90943). * test(reply): use a core-local stub provider instead of the bundled Mattermost import The reconcile-thread regression test deep-imported extensions/mattermost from a core test, which trips the core/extension package boundary (boundary-invariants "keeps core tests off bundled extension deep imports", extension-test-boundary, and check-tsgo-core-boundary pulling extensions/mattermost transitively). Replace it with a core-local channel test plugin that reproduces the same contract: an implicit-threading extractToolSend, a partial extractToolSendResult that reports only { to, threadId? }, and no targetsMatchForReplySuppression matcher. The test now exercises the generic reconciler contract with no extension dependency. It still fails on pristine main and passes with the fix. * fix(reply): reconcile thread evidence atomically --------- Co-authored-by: Vincent Koc <[email protected]>

… send results (openclaw#93291) * fix(reply): preserve pending thread evidence when reconciling partial send results extractMessagingToolSendResult re-derived threadId/threadImplicit/threadSuppressed straight from the provider result. Mattermost is the only production provider that implements extractToolSendResult, and for an implicitly threaded send it reports only { to }, so the reconciler overwrote the correct pending thread evidence with undefined. That defeated same-thread reply suppression in reply-payloads dedupe and delivered the agent's final reply twice in the thread, on both the native and Codex harnesses. A partial provider result now keeps the pending thread evidence it does not speak to: a provider-reported threadId still wins (and clears the implicit flag), but an absent one no longer erases the pending threadId/threadImplicit/threadSuppressed. Regression introduced by c67dc59 (openclaw#90943). * test(reply): use a core-local stub provider instead of the bundled Mattermost import The reconcile-thread regression test deep-imported extensions/mattermost from a core test, which trips the core/extension package boundary (boundary-invariants "keeps core tests off bundled extension deep imports", extension-test-boundary, and check-tsgo-core-boundary pulling extensions/mattermost transitively). Replace it with a core-local channel test plugin that reproduces the same contract: an implicit-threading extractToolSend, a partial extractToolSendResult that reports only { to, threadId? }, and no targetsMatchForReplySuppression matcher. The test now exercises the generic reconciler contract with no extension dependency. It still fails on pristine main and passes with the fix. * fix(reply): reconcile thread evidence atomically --------- Co-authored-by: Vincent Koc <[email protected]>

openclaw-barnacle Bot added agents Agent runtime and tooling proof: supplied External PR includes structured after-fix real behavior proof. size: S labels Jun 15, 2026

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 15, 2026

vincentkoc self-assigned this Jun 16, 2026

yetval and others added 3 commits June 16, 2026 14:50

fix(reply): reconcile thread evidence atomically

3099983

vincentkoc force-pushed the fix/mattermost-reconcile-thread-evidence branch from ca07847 to 3099983 Compare June 16, 2026 07:05

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 16, 2026

vincentkoc merged commit 7fc124d into openclaw:main Jun 16, 2026
161 of 167 checks passed

github-actions Bot mentioned this pull request Jun 16, 2026

📡 Upstream Digest — 2026-06-16 09:19 UTC curtismercier/openclaw-mods#1087

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(reply): preserve pending thread evidence when reconciling partial send results#93291

fix(reply): preserve pending thread evidence when reconciling partial send results#93291
vincentkoc merged 3 commits into
openclaw:mainfrom
yetval:fix/mattermost-reconcile-thread-evidence

yetval commented Jun 15, 2026

Uh oh!

clawsweeper Bot commented Jun 15, 2026 •

edited

Loading

Uh oh!

yetval commented Jun 15, 2026

Uh oh!

clawsweeper Bot commented Jun 15, 2026 •

edited

Loading

Uh oh!

vincentkoc commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

yetval commented Jun 15, 2026

Summary

Root cause

Fix

Why not one-sided

Verification

Real behavior proof

Uh oh!

clawsweeper Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yetval commented Jun 15, 2026

Uh oh!

clawsweeper Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentkoc commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented Jun 15, 2026 •

edited

Loading

clawsweeper Bot commented Jun 15, 2026 •

edited

Loading