Skip to content

fix(msteams): detect provider-prefixed target ids#104364

Closed
VectorPeak wants to merge 3 commits into
openclaw:mainfrom
VectorPeak:fix/msteams-provider-prefixed-target-id
Closed

fix(msteams): detect provider-prefixed target ids#104364
VectorPeak wants to merge 3 commits into
openclaw:mainfrom
VectorPeak:fix/msteams-provider-prefixed-target-id

Conversation

@VectorPeak

@VectorPeak VectorPeak commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #104381

What Problem This Solves

Fixes an issue where provider-prefixed Microsoft Teams explicit targets could be classified differently from the same target after MSTeams normalization.

looksLikeMSTeamsTargetId() already recognized user:<aad-guid> and conversation:<id> as explicit Microsoft Teams targets, and the surrounding MSTeams target helpers already strip local teams: / msteams: prefixes before parsing targets. However, this detector only trimmed the raw input, so teams:user:<aad-guid>, msteams:user:<aad-guid>, and provider-prefixed conversation targets were not classified like their normalized forms.

Why This Change Was Made

This keeps the MSTeams explicit-target detector aligned with the rest of the MSTeams target parsing surface by stripping the local provider prefix before applying the existing ID checks.

The change is intentionally narrow: provider-prefixed AAD user IDs and conversation IDs are classified like their unprefixed forms, while display-name targets such as teams:user:Alice Example still return false so directory lookup behavior is preserved.

User Impact

Users and operators can use provider-prefixed MSTeams explicit targets consistently in paths that rely on the shared target-ID detector. For example, teams:user:<aad-guid> now follows the same explicit-ID classification as user:<aad-guid>, and teams:conversation:19:[email protected] follows the same classification as conversation:19:[email protected].

Evidence

  • Added focused regression coverage showing teams:user:<aad-guid>, msteams:user:<aad-guid>, teams:conversation:19:[email protected], and msteams:conversation:19:[email protected] are recognized as explicit IDs by looksLikeMSTeamsTargetId().
  • Added negative coverage showing teams:user:John Smith and msteams:user:John Smith still do not pass the id-like detector, preserving the existing directory/display-name path.
  • Source-backed user-entry call chain checked on the current branch:
gateway send request
  -> src/gateway/server-methods/send.ts: maybeResolveIdLikeTarget(...)
  -> src/infra/outbound/target-id-resolution.ts: maybeResolvePluginMessagingTarget(...)
  -> src/infra/outbound/target-normalization.ts: looksLikeTargetId(...)
  -> extensions/msteams/src/channel.ts: msteamsPlugin.messaging.targetResolver.looksLikeId
  -> extensions/msteams/src/resolve-allowlist.ts: looksLikeMSTeamsTargetId(...)
  • extensions/msteams/src/channel.ts wires the real MSTeams plugin with targetPrefixes: ["msteams", "teams"], normalizeTarget: normalizeMSTeamsMessagingTarget, and targetResolver.looksLikeId: (raw) => looksLikeMSTeamsTargetId(raw), so this helper is the provider-owned classifier used by the user-facing send target path.
  • node scripts/run-vitest.mjs run extensions/msteams/src/resolve-allowlist.test.ts passes with 43 tests after rebasing onto current main.
  • git diff --check -- extensions/msteams/src/resolve-allowlist.ts extensions/msteams/src/resolve-allowlist.test.ts passes.

Live-proof note: I checked this workstation for a runnable MSTeams live setup before updating the PR evidence. There are no MSTEAMS / MS_TEAMS / TEAMS / OPENCLAW environment keys and no MSTeams channel credentials or targets under the local .openclaw config scan, so I cannot honestly attach a redacted real Microsoft Teams send log from this machine. I am not treating the source-backed call chain above as live-send proof.

Adjacent extensions/msteams/src/channel.directory.test.ts and extensions/msteams/src/channel.test.ts were also considered, but this checkout is missing generated/workspace gateway-protocol package metadata/subpath exports, so those suites fail before running tests and are not treated as evidence for this PR.

@openclaw-barnacle openclaw-barnacle Bot added channel: msteams Channel integration: msteams size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. size: S and removed size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 11, 2026
@clawsweeper

clawsweeper Bot commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Summary
The PR strips teams: and msteams: prefixes inside Microsoft Teams explicit target-ID detection and adds direct positive and negative classifier tests.

PR surface: Source 0, Tests +15. Total +15 across 2 files.

Reproducibility: yes. at source level: current main computes and supplies a normalized target, while the Microsoft Teams callback classifies only the raw provider-prefixed value. A live Microsoft Teams delivery reproduction was not performed.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #104381
Summary: This PR is the direct candidate fix for the linked canonical Microsoft Teams resolver-gate bug.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P1] Use the resolver-provided normalized target and add resolver-gate coverage.
  • Post a redacted live Microsoft Teams send, resolver transcript, or runtime log without private endpoints, credentials, tenant data, or user identifiers.

Proof guidance:

  • [P1] Needs real behavior proof before merge: No after-fix live Microsoft Teams send, resolver transcript, or runtime log is provided; add redacted evidence, update the PR body to trigger review, or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] Provider-prefix handling would have two owners—the shared plugin normalizer and the Microsoft Teams classifier—allowing future changes to classify the same target differently across entry points.
  • [P1] The ID-like predicate gates whether Microsoft Teams target resolution runs, so divergence can suppress intended delivery or send display-name targets through the wrong path.
  • [P1] No real Microsoft Teams after-fix evidence confirms that provider-prefixed explicit targets resolve successfully while prefixed display names retain directory lookup behavior.

Maintainer options:

  1. Repair the callback boundary (recommended)
    Consume the normalized callback target, add resolver-gate tests for explicit IDs and display names, and obtain real Microsoft Teams proof before merge.
  2. Pause pending contributor follow-up
    Keep the PR open but unmerged until the repeated P1 finding and environment-specific proof requirement are resolved.

Next step before merge

  • [P1] The code-and-test defect has a narrow mechanical repair on the PR branch, while real Microsoft Teams proof still requires contributor or maintainer access to an appropriate environment.

Security
Cleared: The focused TypeScript classifier and test diff introduces no concrete security or supply-chain concern.

Review findings

  • [P1] Use the normalized target supplied by the resolver — extensions/msteams/src/resolve-allowlist.ts:186
Review details

Best possible solution:

Use normalized ?? raw in the Microsoft Teams looksLikeId callback, keep looksLikeMSTeamsTargetId limited to canonical target forms, add resolver-boundary regression coverage, and provide a redacted live Microsoft Teams send or resolver log.

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

Yes, at source level: current main computes and supplies a normalized target, while the Microsoft Teams callback classifies only the raw provider-prefixed value. A live Microsoft Teams delivery reproduction was not performed.

Is this the best way to solve the issue?

No. Consuming the resolver's existing normalized callback argument is narrower, follows the plugin contract, and avoids introducing a second provider-prefix normalization path.

Full review comments:

  • [P1] Use the normalized target supplied by the resolver — extensions/msteams/src/resolve-allowlist.ts:186
    Classify the callback's normalized argument instead of teaching this helper to repeat provider-prefix stripping. Core already invokes looksLikeId(raw, normalized) after plugin normalization, and a sibling channel follows that contract; duplicating it here can drift and leaves the actual boundary untested. Update the Microsoft Teams registration and cover prefixed explicit IDs plus display names through the resolver gate.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR addresses a bounded Microsoft Teams outbound target-resolution bug with limited blast radius.
  • merge-risk: 🚨 message-delivery: The modified classifier determines whether provider-prefixed Microsoft Teams targets proceed to resolution and delivery.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No after-fix live Microsoft Teams send, resolver transcript, or runtime log is provided; add redacted evidence, update the PR body to trigger review, or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source 0, Tests +15. Total +15 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 1 0
Tests 1 21 6 +15
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 22 7 +15

Acceptance criteria:

  • [P1] pnpm test extensions/msteams/src/resolve-allowlist.test.ts extensions/msteams/src/channel.test.ts src/infra/outbound/target-normalization.test.ts.
  • [P1] pnpm check:changed -- extensions/msteams/src/channel.ts extensions/msteams/src/resolve-allowlist.ts extensions/msteams/src/resolve-allowlist.test.ts extensions/msteams/src/channel.test.ts src/infra/outbound/target-normalization.test.ts.
  • [P1] git diff --check -- extensions/msteams/src/channel.ts extensions/msteams/src/resolve-allowlist.ts extensions/msteams/src/resolve-allowlist.test.ts extensions/msteams/src/channel.test.ts src/infra/outbound/target-normalization.test.ts.

What I checked:

Likely related people:

  • steipete: Substantive merged refactors and fixes shaped the Microsoft Teams allowlist, directory, and target-resolution module. (role: feature owner; confidence: high; commits: 7230b96cc7de, 8c1afc4b63fe, 18acfe735239; files: extensions/msteams/src/resolve-allowlist.ts)
  • sudie-codes: Merged work added Microsoft Teams conversation-ID recognition in the same explicit-target classifier area. (role: adjacent owner; confidence: high; commits: 11f924ba049f; files: extensions/msteams/src/resolve-allowlist.ts)
  • vincentkoc: Release-tag blame attributes the shipped detector and its display-name fallback contract to this contributor's visible history boundary. (role: introduced shipped behavior; confidence: medium; commits: e085fa1a3ffd; files: extensions/msteams/src/resolve-allowlist.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 (6 earlier review cycles)
  • reviewed 2026-07-11T10:07:57.594Z sha eacadb8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-11T10:16:25.988Z sha eacadb8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-11T10:24:57.096Z sha eacadb8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-11T10:50:48.695Z sha a8a77e4 :: needs real behavior proof before merge. :: [P1] Use the normalized target supplied by the resolver
  • reviewed 2026-07-11T10:57:57.500Z sha 8aed6c7 :: needs real behavior proof before merge. :: [P1] Use the normalized target supplied by the resolver
  • reviewed 2026-07-11T11:06:17.630Z sha 57ec5ae :: needs real behavior proof before merge. :: [P1] Use the normalized target supplied by the resolver

@VectorPeak VectorPeak mentioned this pull request Jul 11, 2026
Open
@openclaw-barnacle openclaw-barnacle Bot added triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 11, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 11, 2026
@VectorPeak
VectorPeak force-pushed the fix/msteams-provider-prefixed-target-id branch from eacadb8 to 5531eaf Compare July 11, 2026 10:32
@VectorPeak
VectorPeak force-pushed the fix/msteams-provider-prefixed-target-id branch from 5531eaf to 712f1ed Compare July 11, 2026 10:34
@VectorPeak

VectorPeak commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Update after the reviewer proof request:

  • Rebased the branch onto current upstream/main and force-pushed the refreshed PR head.
  • Kept the code diff minimal: one production-line behavior change in extensions/msteams/src/resolve-allowlist.ts plus focused adjacent helper coverage in extensions/msteams/src/resolve-allowlist.test.ts.
  • Removed the attempted shared resolver-gate test from the branch because it pulled in the full MSTeams/gateway-protocol import path and cannot run in this checkout.
  • Updated the PR body to replace the stale mocked resolver-gate evidence with a source-backed user-entry call chain:
gateway send request
  -> maybeResolveIdLikeTarget(...)
  -> maybeResolvePluginMessagingTarget(...)
  -> looksLikeTargetId(...)
  -> msteamsPlugin.messaging.targetResolver.looksLikeId
  -> looksLikeMSTeamsTargetId(...)

Validation after the latest rebase:

node scripts/run-vitest.mjs run extensions/msteams/src/resolve-allowlist.test.ts
# 1 file passed, 43 tests passed

git diff --check -- extensions/msteams/src/resolve-allowlist.ts extensions/msteams/src/resolve-allowlist.test.ts
# passed

Live-proof limitation: I checked this workstation for a runnable Microsoft Teams setup before updating the evidence. There are no MSTEAMS / MS_TEAMS / TEAMS / OPENCLAW environment keys and no MSTeams channel credentials or targets in the local .openclaw config scan, so I cannot honestly attach a redacted real Teams send log from this machine. The PR body now states that limitation directly instead of treating source-chain evidence as live-send proof.

@clawsweeper

clawsweeper Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(msteams): detect provider-prefixed target ids This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@VectorPeak @chatgpt-codex-connector
fix(msteams): detect provider-prefixed target ids
a8a77e4 
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
@VectorPeak
VectorPeak force-pushed the fix/msteams-provider-prefixed-target-id branch from 712f1ed to a8a77e4 Compare July 11, 2026 10:43
@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. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 11, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. label Jul 11, 2026
@VectorPeak

VectorPeak commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Closing this PR for now because the requested live Microsoft Teams behavior proof is not available from this environment, and I do not want to over-claim evidence. Thanks again for the review and guidance.

@VectorPeak VectorPeak closed this Jul 11, 2026
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. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS 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]: MSTeams provider-prefixed explicit target IDs miss resolver gate

1 participant

@VectorPeak