Skip to content

fix(msteams): emit message:sent hook on reply delivery#82354

Open
ndholakia wants to merge 3 commits into
openclaw:mainfrom
ndholakia:fix/msteams-message-sent-hook
Open

fix(msteams): emit message:sent hook on reply delivery#82354
ndholakia wants to merge 3 commits into
openclaw:mainfrom
ndholakia:fix/msteams-message-sent-hook

Conversation

@ndholakia

@ndholakia ndholakia commented May 15, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

Closes a parity gap with telegram: the msteams reply-dispatcher
previously bypassed both the internal hook bus AND the plugin-SDK
message_sent hook entirely on outbound delivery. Downstream listeners
(audit-loggers, per-user memory substrates) cannot observe agent replies
on msteams as a result.

Telegram emits both correctly via extensions/telegram/src/bot/delivery.replies.ts's
emitTelegramMessageSentHooks (mirror of emitInternalMessageSentHook

  • emitMessageSentHooks). msteams had no equivalent module — this PR
    adds it.

Files

  • NEW extensions/msteams/src/delivery-hooks.ts — mirrors the
    telegram pattern:

    • buildMSTeamsSentHookContext() with channelId: "msteams" AND
      sessionKey populated from sessionKeyForInternalHooks so
      plugin-SDK consumers can correlate
    • emitInternalMessageSentHook() gated on
      sessionKeyForInternalHooks
    • emitMessageSentHooks() fans out to both the plugin-SDK
      message_sent runner and the internal bus
    • emitMSTeamsMessageSentHooks() is the public entry point that
      resolves the global hook runner

  • MODIFIED extensions/msteams/src/reply-dispatcher.ts —
    flushPendingMessages now calls emitMSTeamsMessageSentHooks after
    the delivery attempt completes. The to field resolves to the
    recipient AAD for personal DMs (matching telegram's chatId
    semantics) and falls back to the conversation id for group/channel
    surfaces. The failure-tracking state was refactored to span both the
    batch and per-message retry paths so success / error /
    messageId are reported consistently.

  • MODIFIED extensions/msteams/src/reply-dispatcher.test.ts —
    three new test cases:

    • Personal-DM success path → asserts hook called with correct
      to / success / messageId / isGroup=false
    • GroupChat success path → asserts isGroup=true + groupId
    • Full failure path → asserts success=false + error populated +
      messageId undefined

Behavior

Internal hook emission is conditional on sessionKeyForInternalHooks
being set (same gate telegram uses). Plugin-SDK message_sent hook
emission is conditional on hookRunner.hasHooks("message_sent"). Hook
handlers can use the isGroup + groupId flags to disambiguate
personal DMs from group/channel surfaces.

Real behavior proof (redacted production logs)

Verified end-to-end against a downstream consumer in production (an
internal-hook handler subscribed to ["message:sent"] that POSTs to
an audit endpoint).

Before this patch — same downstream handler registered for the
catch-all ["message"] namespace, real Teams DM round-trip:

2026-05-15T22:09:10.002Z [log-outbound-dm] event 1/3:
  {"probe":1,"type":"message","action":"received","sessionKey":"agent:eva:msteams:direct:<REDACTED-AAD>","channelId":"msteams",...}
2026-05-15T22:09:10.061Z [log-outbound-dm] event 2/3:
  {"probe":2,"type":"message","action":"preprocessed","sessionKey":"agent:eva:msteams:direct:<REDACTED-AAD>","channelId":"msteams",...}
[no message:sent event for the corresponding reply — the bot's reply DID reach Teams, but the hook bus was never notified]

grep -rn "triggerInternal\|emitInternal" extensions/msteams/src/ on
the pre-patch source returned zero results, confirming the gap is
structural.

After this patch — same handler with ["message:sent"]
registration, real Teams DM round-trip on the patched gateway:

2026-05-16T01:52:17.473Z [log-outbound-dm] event 1/3:
  {"probe":1,"type":"message","action":"received","sessionKey":"agent:eva:msteams:direct:<REDACTED-AAD>","channelId":"msteams",...}
2026-05-16T01:52:17.508Z [log-outbound-dm] event 2/3:
  {"probe":2,"type":"message","action":"preprocessed","sessionKey":"agent:eva:msteams:direct:<REDACTED-AAD>","channelId":"msteams",...}
2026-05-16T01:52:47.452Z [log-outbound-dm] event 3/3:
  {"probe":3,"type":"message","action":"sent","sessionKey":"agent:eva:msteams:direct:<REDACTED-AAD>","channelId":"msteams","contextKeys":["accountId","channelId","content","conversationId","isGroup","messageId","success","to"]}
2026-05-16T01:52:47.454Z [log-outbound-dm] SENT detail:
  {"type":"message","action":"sent","channelId":"msteams","toType":"string","toPreview":"<REDACTED-AAD>","contentLen":686,"success":true,...}
2026-05-16T01:52:47.473Z DM logged via /surreal/log-dm
  {"direction":"outbound","user":"<REDACTED-AAD>","context":"personal","intent":"reply"}
2026-05-16T01:52:47.686Z [log-outbound-dm] SENT detail:
  {"type":"message","action":"sent","channelId":"msteams","toType":"string","toPreview":"<REDACTED-AAD>","contentLen":576,"success":true,...}
2026-05-16T01:52:47.707Z DM logged via /surreal/log-dm
  {"direction":"outbound","user":"<REDACTED-AAD>","context":"personal","intent":"reply"}

Observations:

  • message:sent fires exactly twice — once for the stream-finalize
    emit (contentLen=686) and once for a follow-up flushPendingMessages
    emit (contentLen=576). Both report success=true with the recipient
    AAD as to (matches telegram's chatId semantics for personal DMs).
  • The downstream handler's HTTP POSTs complete within ~21ms of the
    hook firing.

How this was discovered

Internal-hook handler registered for the message namespace caught
message:received and message:preprocessed events for msteams but
never message:sent. grep -rn "triggerInternal\|emitInternal\|hookRunner\|message_sent" extensions/msteams/src/
on the pre-patch source returned zero matches, confirming the gap is
architectural rather than configuration. The fix matches the telegram
pattern verbatim modulo the channel-specific recipient identifier
semantics.

Test plan

  • pnpm exec vitest run extensions/msteams/src/reply-dispatcher.test.ts passes
  • Real Teams DM round-trip captured (see "Real behavior proof" above)
  • CI passes
  • Reviewer confirms no behavioral regression on existing
    reply-dispatcher.test.ts cases

ndholakia added 2 commits May 15, 2026 17:45
@ndholakia
fix(msteams): emit message:sent hook on reply delivery
feb5ddd 
The msteams reply-dispatcher previously bypassed both the internal hook
bus and the plugin-SDK message_sent hook entirely on outbound delivery,
leaving downstream listeners (per-user memory loggers, audit substrates)
unable to observe agent replies. Telegram emits both correctly via
extensions/telegram/src/bot/delivery.replies.ts:emitTelegramMessageSentHooks;
msteams had no equivalent module.

This patch mirrors the telegram pattern:

  NEW extensions/msteams/src/delivery-hooks.ts
    - buildMSTeamsSentHookContext(): canonical context with channelId="msteams"
    - emitInternalMessageSentHook(): fires triggerInternalHook("message","sent",...)
      gated on sessionKeyForInternalHooks
    - emitMessageSentHooks(): fans out to both plugin-SDK and internal buses
    - emitMSTeamsMessageSentHooks(): public entry point that resolves the
      global hook runner and computes enabled state

  MODIFIED extensions/msteams/src/reply-dispatcher.ts
    - Calls emitMSTeamsMessageSentHooks at the end of flushPendingMessages,
      after the (possibly partial) delivery attempt completes
    - `to` resolves to recipient AAD for personal DMs (matches telegram's
      chatId semantics) and falls back to conversationId for group/channel
    - Tracks failureCount + lastError across the batch + per-message retry
      paths so the hook reports success/error consistently
    - Concatenates rendered text content for the hook payload

Hook handlers can use the `isGroup` + `groupId` flags to disambiguate
personal DMs from group/channel surfaces.

Tests: deferred (existing reply-dispatcher.test.ts scaffolding already
mocks hookRunner; will add explicit assertions in a follow-up commit
before upstream PR submission).

Validated against a real Teams DM round-trip: prior log-outbound-dm hook
(events: ["message"]) captured message:received and message:preprocessed
but never message:sent for msteams. CloudWatch evidence in the EVA
project's session notes.
@ndholakia
test(msteams): assert message:sent hook fires on reply delivery
26b1178 
Adds three vitest cases to reply-dispatcher.test.ts:

1. Personal-DM success path → emitMSTeamsMessageSentHooks called once
   with channelId-implicit context (provider hardcodes "msteams"),
   to=user.aadObjectId, success=true, messageId set, isGroup=false,
   error undefined.

2. Group chat success path → to=conversationId, isGroup=true,
   groupId=conversationId.

3. Full failure path (initial batch throws + per-message retries all
   throw) → success=false, error populated, messageId undefined.

The hook module is mocked via vi.mock("./delivery-hooks.js") so the
test asserts the dispatcher's call shape rather than the downstream
triggerInternalHook plumbing (covered by hook-runtime's own tests).
@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: M triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 15, 2026
@socket-security

socket-security Bot commented May 15, 2026 •
edited
Loading

Copy link
Copy Markdown

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

@clawsweeper

clawsweeper Bot commented May 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 30, 2026, 9:23 PM ET / July 1, 2026, 01:23 UTC.

Summary
Adds an MS Teams delivery-hook helper, calls it from queued reply delivery, and adds dispatcher tests for personal, group, and failed queued sends.

PR surface: Source +170, Tests +170. Total +340 across 3 files.

Reproducibility: yes. Source inspection on current main shows personal-DM streaming can suppress block delivery and then finalize successfully without running the PR's queued-send hook path.

Review metrics: 1 noteworthy metric.

  • Hook-path coverage: 3 queued-block tests added, 0 native-stream hook tests added. Personal MS Teams DMs use native streaming by default, so queued-only tests do not prove every successful reply delivery is observable.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Rebase onto current main so maintainers can review the exact MS Teams stream-controller merge result.
  • [P2] Add sent-hook emission and focused coverage for successful native stream finalization.

Proof path suggestion
A generic live task could usefully prove both queued block delivery and native-streamed MS Teams personal-DM hook emission. Mantis is currently scoped to Telegram, Discord, and web UI chat proof, so it is not the right proof path for this surface.
Use maintainer screenshot/manual proof, browser or Playwright proof, Crabbox where appropriate, or normal local artifact proof instead.

Risk before merge

  • [P1] Successful native-streamed personal-DM replies can still complete without plugin message_sent or internal message:sent observation.
  • [P1] The branch is currently conflicting with current main, so maintainers do not have a clean exact merge result for the MS Teams stream-controller path.

Maintainer options:

  1. Cover Native Stream Finalization (recommended)
    Add sent-hook emission to successful native stream close/finalization while preserving queued block-send hooks, then rebase and re-review the exact head.
  2. Pause For Contributor Refresh
    Leave the PR open for the contributor to rebase and add native-stream hook coverage before maintainer review resumes.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Rebase the PR onto current main and add MS Teams sent-hook emission for successful native stream finalization while preserving the queued block-send hook, with targeted tests in reply-dispatcher and reply-stream-controller.

Next step before merge

  • [P2] A bounded repair can rebase the branch and route the same hook helper through native stream finalization without a product decision.

Security
Cleared: The live diff is limited to MS Teams TypeScript runtime/tests and adds no dependencies, workflows, lockfile changes, secrets handling, or third-party execution.

Review findings

  • [P2] Emit sent hooks from native stream finalization — extensions/msteams/src/reply-dispatcher.ts:279-290
Review details

Best possible solution:

Rebase and emit one canonical MS Teams sent-hook outcome from both queued block sends and successful native stream finalization, with focused tests for each path.

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

Yes. Source inspection on current main shows personal-DM streaming can suppress block delivery and then finalize successfully without running the PR's queued-send hook path.

Is this the best way to solve the issue?

No. The helper shape is reasonable, but the call site is too narrow; Slack's equivalent finalizer emits after successful stream close to cover the streaming happy path.

Full review comments:

  • [P2] Emit sent hooks from native stream finalization — extensions/msteams/src/reply-dispatcher.ts:279-290
    This hook only runs after queued block sends. Personal Teams DMs use native streaming by default, and preparePayload can return undefined after streamed text so markDispatchIdle finalizes the stream without queued delivery; observers still miss message_sent / message:sent for that successful path.
    Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded MS Teams hook-observability bug with limited channel scope while visible message delivery still works.
  • merge-risk: 🚨 message-delivery: The current diff can still let a real outbound MS Teams reply path complete without the expected sent-hook delivery event.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (logs): The PR body includes redacted before/after production MS Teams logs showing message:sent after the patch for a real DM, though source review still finds a native-stream gap.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted before/after production MS Teams logs showing message:sent after the patch for a real DM, though source review still finds a native-stream gap.
Evidence reviewed

PR surface:

Source +170, Tests +170. Total +340 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 179 9 +170
Tests 1 170 0 +170
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 349 9 +340

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs extensions/msteams/src/reply-dispatcher.test.ts extensions/msteams/src/reply-stream-controller.test.ts.
  • [P1] gh pr checks 82354 --repo openclaw/openclaw.

What I checked:

Likely related people:

  • heyitsaamir: Authored merged PR fix(msteams): rebase TeamsSDK patterns to simplify Teams Integration #76262, which rewrote the MS Teams dispatcher and native stream-controller surfaces this PR must integrate with. (role: MS Teams SDK migration contributor; confidence: high; commits: 04c29825356f, 151bb6e16d0a, a3878942d5df; files: extensions/msteams/src/reply-dispatcher.ts, extensions/msteams/src/reply-stream-controller.ts, extensions/msteams/src/reply-stream-controller.test.ts)
  • steipete: Merged fix(msteams): rebase TeamsSDK patterns to simplify Teams Integration #76262 and has follow-up commits in the same MS Teams SDK migration stream and adjacent channel delivery code. (role: merger and adjacent area contributor; confidence: medium; commits: 04c29825356f, 3f862bbac0dd, c88fd2fdb3b8; files: extensions/msteams/src/reply-dispatcher.ts, extensions/msteams/src/reply-stream-controller.ts)
  • Brad Groux: Introduced blockStreaming/progressive delivery behavior that is part of the delivery surface affected by hook coverage. (role: progressive delivery contributor; confidence: medium; commits: 6b0e74000d9f; files: extensions/msteams/src/reply-dispatcher.ts, extensions/msteams/src/reply-stream-controller.ts)
  • Yuval Dinodia: Current main blame for the dispatcher and stream-controller lines maps to commit 82871fe, though the broad commit title makes ownership ambiguous. (role: current-line contributor; confidence: low; commits: 82871fe21b3b; files: extensions/msteams/src/reply-dispatcher.ts, extensions/msteams/src/reply-stream-controller.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.

@yfge

This comment was marked as spam.

Sign in to view
@ndholakia
fix(msteams): include sessionKey in canonical hook context
419da85 
Addresses ClawSweeper review finding [P2] on PR openclaw#82354. Pass
`params.sessionKeyForInternalHooks` as the `sessionKey` field of the
canonical hook context built in `buildMSTeamsSentHookContext`, not
just as the internal-hook routing key.

Telegram's `buildTelegramSentHookContext` does not include sessionKey
either — but the canonical context type accepts it, and the plugin-SDK
consumers reading the canonical/plugin context object (via
`toPluginMessageContext`) can use it to correlate the sent event with
the originating agent session. Including it costs nothing.
@ndholakia
ndholakia force-pushed the fix/msteams-message-sent-hook branch from 5e31d7d to 419da85 Compare May 16, 2026 02:48
@ndholakia

ndholakia commented May 16, 2026 •
edited by clawsweeper Bot
Loading

Copy link
Copy Markdown
Author

Thanks @yfge and @clawsweeper — both findings addressed in the rebased branch:

  1. [P1] Fork-only ECR workflow removed. Dropped the ci(fork): build + push OpenClaw base image commit from this branch entirely via rebase. .github/workflows/eva-fork-build.yml is no longer in the PR diff; the file lives only on our internal eva/\* branch which doesn't target upstream.

  2. [P2] sessionKey included in canonical hook context. buildMSTeamsSentHookContext now passes params.sessionKeyForInternalHooks as the sessionKey field of the canonical context, so plugin-SDK consumers reading the canonical/plugin context object can correlate the sent event with the originating agent session — not just the internal-hook routing path.

  3. Real behavior proof added to PR body. Redacted production log evidence from a real Teams DM round-trip on the patched gateway, showing the message:sent event firing twice (stream-finalize emit + flushPendingMessages emit) with success=true, both within ~30s of the user's inbound message. Pre-patch evidence from the same gateway shows message:received and message:preprocessed firing but no message:sent — confirming the gap is closed.

Branch rebased to 419da85f07 on top of upstream main ea16a5e9e1. Force-pushed.

@clawsweeper re-review

Re-review progress:

@clawsweeper

clawsweeper Bot commented May 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 16, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 16, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 16, 2026
@BradGroux BradGroux mentioned this pull request May 16, 2026
Draft
@giodl73-repo giodl73-repo removed the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 21, 2026
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 21, 2026
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 21, 2026
@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🥚 Incubating: this PR egg is tucked into the review nest.

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.
What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

This was referenced Jun 11, 2026
Open
Closed
Closed
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 14, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label Jun 14, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. label Jun 15, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 27, 2026
Open
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 merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ndholakia @yfge @giodl73-repo