Skip to content

fix: prevent duplicate Teams broker retry turns#107171

Open
galiniliev wants to merge 1 commit into
openclaw:mainfrom
galiniliev:bug-056-msteams-delivery-dedupe
Open

fix: prevent duplicate Teams broker retry turns#107171
galiniliev wants to merge 1 commit into
openclaw:mainfrom
galiniliev:bug-056-msteams-delivery-dedupe

Conversation

@galiniliev

@galiniliev galiniliev commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Closes #107170

What Problem This Solves

Fixes an issue where Microsoft Teams users could see duplicate agent replies or repeated side effects when a brokered Teams webhook delivery is retried while the first delivery is in flight or within the retry window after success.

Why This Change Was Made

The Teams monitor now projects the broker delivery header into the activity before SDK dispatch and uses the shared persistent claimable dedupe helper around both Teams agent-dispatch paths. The guard commits only after successful dispatch, releases failed claims so retries remain eligible, scopes fallback activity keys by source and Teams context, and skips fallback suppression when no stable event identity exists.

User Impact

Teams broker retries with the same stable delivery id no longer start duplicate agent turns. Normal Teams events without a stable delivery id continue to dispatch rather than being guessed into a dedupe key.

Evidence

Before evidence:

rg -n "x-teams-delivery-id|lobsterDeliveryId|createBrokeredDeliveryDedupe|runBrokeredDeliveryOnce" extensions/msteams/src
# no matches on current main

After-fix focused test output:

node scripts/run-vitest.mjs extensions/msteams/src/monitor.lifecycle.test.ts
[test] starting test/vitest/vitest.extension-msteams.config.ts

 RUN  v4.1.9 <repo>

 Test Files  1 passed (1)
      Tests  24 passed (24)
   Duration  3.94s

[test] passed 1 Vitest shard in 9.02s

Patch hygiene:

git diff --check -- extensions/msteams/src/monitor.ts extensions/msteams/src/monitor.lifecycle.test.ts
# passed with no output

Structured review:

autoreview --mode commit --commit HEAD
autoreview clean: no accepted/actionable findings reported
overall: patch is correct (0.82)

Real behavior proof

Behavior addressed: Microsoft Teams brokered webhook retries with the same stable delivery id can no longer start duplicate agent turns through the monitor dispatch path.

Real environment tested: Local repo checkout with focused Microsoft Teams monitor lifecycle tests. The tests exercise the real monitor registration and dispatch wrapper with mocked Teams SDK routes and mocked persistent dedupe factory to avoid writing test state to the user database.

Exact steps or command run after this patch: node scripts/run-vitest.mjs extensions/msteams/src/monitor.lifecycle.test.ts and git diff --check -- extensions/msteams/src/monitor.ts extensions/msteams/src/monitor.lifecycle.test.ts.

Evidence after fix: Terminal capture above shows the focused Teams lifecycle suite passed with 24 tests and patch whitespace check passed with no output.

Observed result after fix: The new tests prove duplicate non-poll card actions are suppressed while in flight and after success, activity-id fallback dedupes only stable scoped events, in-flight retries process after owner failure, message updates are not suppressed by bare message id, and events without a stable id are not suppressed.

What was not tested: No live Microsoft Teams broker replay was run; the local bug evidence did not include credentials or a raw duplicate live Teams turn log. Remote Testbox was unavailable in the initial environment because the installed Crabbox wrapper was too old.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: L maintainer Maintainer-authored PR labels Jul 14, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 14, 2026
Open
@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 Jul 14, 2026
@clawsweeper

clawsweeper Bot commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 14, 2026, 7:37 AM ET / 11:37 UTC.

Summary
The PR projects a Teams broker delivery identifier into activities, wraps both Teams agent-dispatch paths with shared persistent claim-based deduplication, and expands monitor lifecycle regression tests.

PR surface: Source +197, Tests +481. Total +678 across 2 files.

Reproducibility: yes. from source at medium confidence: current main has two direct Teams agent-dispatch paths without delivery dedupe, but no live broker replay establishes the exact production retry identity. citeturn533451view0.

Review metrics: 1 noteworthy metric.

  • Dispatch guards: 2 paths guarded. Both non-poll card actions and catch-all activities can start agent work and must share the same retry invariant.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #107170
Summary: This PR is the candidate fix for the linked Microsoft Teams duplicate-turn issue.

Members:

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

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
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:

  • [P1] Add redacted broker replay output showing one turn for repeated stable delivery ids and a successful retry after a failed owner dispatch.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Evidence is limited to mocked lifecycle tests; add redacted live or faithful broker replay logs or terminal output, update the PR body to trigger review, or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] No live or faithful replayed Microsoft Teams broker run demonstrates that repeated deliveries produce exactly one agent turn.
  • [P2] The PR does not independently prove that x-teams-delivery-id is present and stable in the actual broker path; fallback activity identity reduces but does not eliminate that contract uncertainty.

Maintainer options:

  1. Prove the broker path before merge (recommended)
    Keep the implementation but require a redacted live or faithful replay showing duplicate suppression and a retry succeeding after owner failure.
  2. Pause if stable identity is unavailable
    Do not merge until producer-side evidence establishes which identifier remains stable across actual broker retries.

Next step before merge

  • [P1] The next action is contributor-supplied real Teams broker proof followed by maintainer review of the exact head, not an automated code repair.

Security
Cleared: The runtime-and-test change adds no dependency, workflow, permission, secret, or third-party execution surface, and no concrete security regression was found.

Review details

Best possible solution:

Keep one Teams-owned wrapper around both dispatch boundaries using the shared claimable dedupe, key only on proven stable delivery identity, and land after a redacted broker replay confirms duplicate suppression and retry-after-failure behavior.

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

Yes from source at medium confidence: current main has two direct Teams agent-dispatch paths without delivery dedupe, but no live broker replay establishes the exact production retry identity. citeturn533451view0.

Is this the best way to solve the issue?

Yes in shape: a Teams-owned wrapper using the shared claim/commit/release helper is narrower and more maintainable than new dedupe infrastructure, provided the real broker identity and failure path are proven. citeturn429913view0.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: The PR addresses a channel-specific duplicate-turn bug with meaningful but bounded Microsoft Teams impact.
  • merge-risk: 🚨 message-delivery: An incorrect dedupe identity or claim lifecycle could permit duplicate Teams turns or suppress legitimate activities.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Evidence is limited to mocked lifecycle tests; add redacted live or faithful broker replay logs or terminal output, update the PR body to trigger review, or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +197, Tests +481. Total +678 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 201 4 +197
Tests 1 485 4 +481
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 686 8 +678

What I checked:

  • Current-main behavior: The current Teams monitor directly dispatches non-poll card actions and catch-all activities to the handler without a broker delivery claim or recent-delivery check. citeturn533451view0. (extensions/msteams/src/monitor.ts, 9c03b2310e6c)
  • Shared dedupe contract: The existing plugin SDK helper provides claim, in-flight waiting, commit-after-success, release-after-failure, bounded TTL, and plugin-state SQLite persistence, matching the needed ownership semantics. citeturn429913view0. (src/plugin-sdk/persistent-dedupe.ts:18, 9c03b2310e6c)
  • Patch coverage: The branch applies the guard to the two monitor paths that can start agent work and adds focused cases for in-flight duplicates, committed duplicates, failed-owner retry, fallback identity, message updates, and missing identity. (extensions/msteams/src/monitor.lifecycle.test.ts, 3ce965cec0dc)
  • Upstream adapter contract: Microsoft's Teams SDK server integration passes the parsed request body and headers into the route handler, so pre-route body projection is consistent with the dependency's Express integration shape. citeturn424547search0turn424547search1.
  • Proof continuity: The reviewed head is unchanged from the prior ClawSweeper cycle, and the PR explicitly states that no live Microsoft Teams broker replay was run; the added evidence remains mocked test output. (3ce965cec0dc)
  • Feature history: The present monitor structure appears to date to the January 2026 Teams monitor/store split, with later Teams delivery and lifecycle-test maintenance by adjacent contributors. citeturn592640search0turn349009search0turn585540search2. (extensions/msteams/src/monitor.ts, 8875dbd449)

Likely related people:

  • steipete: The current Teams monitor and store split appears to date to the January 2026 refactor series, making this the strongest historical owner signal for the dispatch boundary. citeturn592640search0. (role: introduced monitor structure; confidence: medium; commits: 8875dbd449, 6b107e9e74; files: extensions/msteams/src/monitor.ts)
  • vincentkoc: Recent Microsoft Teams test and plugin-boundary work includes monitor-adjacent fixture and lifecycle maintenance. citeturn349009search0. (role: recent adjacent contributor; confidence: medium; commits: bd1e78ea34; files: extensions/msteams/src/monitor.lifecycle.test.ts)
  • BradGroux: Recent Teams progressive-delivery work touched the same channel behavior area and added focused Teams coverage. citeturn585540search2. (role: recent feature contributor; confidence: medium; commits: 6b0e74000d; files: extensions/msteams/src/monitor.ts, extensions/msteams/src/monitor.lifecycle.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.
Review history (1 earlier review cycle)
  • reviewed 2026-07-14T11:13:03.697Z sha 3ce965c :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 14, 2026
@BradGroux BradGroux mentioned this pull request Jul 14, 2026
Draft
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

channel: msteams Channel integration: msteams maintainer Maintainer-authored PR merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: L status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Teams brokered webhook retries can start duplicate turns

1 participant

@galiniliev