Skip to content

Support separate Teams Graph tenant#87169

Closed
DanBotero wants to merge 1 commit into
openclaw:mainfrom
DanBotero:fix/msteams-graph-tenant-split
Closed

Support separate Teams Graph tenant#87169
DanBotero wants to merge 1 commit into
openclaw:mainfrom
DanBotero:fix/msteams-graph-tenant-split

Conversation

@DanBotero

@DanBotero DanBotero commented May 27, 2026

Copy link
Copy Markdown

Summary

  • add optional channels.msteams.graphTenantId to the config schema and public Teams config type
  • use graphTenantId for Microsoft Graph token acquisition while leaving bot credentials on tenantId
  • document the split and regenerate bundled channel config metadata

Tests

  • node scripts/run-vitest.mjs extensions/msteams/src/graph.test.ts extensions/msteams/src/channel.test.ts
  • pnpm tsgo:core:test && pnpm tsgo:extensions:test
  • pnpm config:channels:check

Coordinated change

Pairs with Botero-Labs/platform#360, which emits graphTenantId for customer tenant Graph/RSC calls while keeping tenantId as the bot home tenant.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: msteams Channel integration: msteams gateway Gateway runtime labels May 27, 2026
@DanBotero

DanBotero commented May 27, 2026

Copy link
Copy Markdown
Author

Coordinated Platform provisioning PR: https://github.com/Botero-Labs/platform/pull/360

@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 11:40 AM ET / 15:40 UTC.

Summary
Adds optional channels.msteams.graphTenantId config/schema/docs/metadata and uses it for Microsoft Teams Graph token acquisition while leaving bot credentials on tenantId.

PR surface: Source +10, Tests +32, Docs +5, Generated 0. Total +47 across 8 files.

Reproducibility: Do we have a high-confidence way to reproduce the issue? Partially: source inspection shows current main has no graphTenantId and always uses the Teams credentials tenant for Graph token setup, but no live split-tenant Teams/Graph deployment proof is available.

Review metrics: 1 noteworthy metric.

  • Teams config surface: 1 optional key added. channels.msteams.graphTenantId changes the operator-facing Teams auth contract and needs maintainer-visible compatibility review.

Stored data model
Persistent data-model change detected: unknown-data-model-change: src/config/bundled-channel-config-metadata.generated.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #87169
Summary: This PR is the current open implementation attempt for split Teams bot tenant versus Graph tenant support; related Teams Graph PRs overlap but do not safely replace it.

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: 🧂 unranked krab
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:

  • Rebase on current main and preserve Teams cloud/serviceUrl plus China Graph guard behavior in the Graph token path.
  • [P1] Add redacted real split-tenant Teams/Graph proof to the PR body; redact tenant IDs, endpoints, API keys, phone numbers, non-public endpoints, and other private details.
  • Get maintainer acceptance for the new channels.msteams.graphTenantId config contract.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists tests and a coordinated provisioning PR, but no redacted live Teams/Graph split-tenant output, terminal log, screenshot, recording, or linked artifact showing the after-fix behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The branch is currently dirty/conflicting, and a naive rebase could keep the PR's loadMSTeamsSdkWithAuth(graphCreds) shape while losing current-main cloud options and the China Graph fail-closed guard.
  • [P1] The new channels.msteams.graphTenantId key changes the operator-facing Teams auth configuration contract and needs maintainer acceptance plus upgrade/docs proof.
  • [P1] No redacted real split-tenant Teams/Graph run is present, so tests prove the unit path but not that a credentialed customer-tenant Graph call works.
  • [P1] The changed tenant-selection path is security-sensitive because it controls which Microsoft tenant and cloud boundary obtains Graph tokens.

Maintainer options:

  1. Rebase and preserve cloud boundaries (recommended)
    Update the PR on current main so graphTenantId only changes the Graph token tenant while keeping SDK cloud options, serviceUrl behavior, and the China Graph guard intact.
  2. Accept the new Teams auth config contract
    Maintainers can explicitly accept the new channels.msteams.graphTenantId surface after docs, generated metadata, and upgrade behavior are clear.
  3. Pause until live tenant proof exists
    Keep the PR unmerged if no credentialed split-tenant Teams/Graph proof can be supplied for the new auth path.

Next step before merge

  • [P1] Needs maintainer acceptance of the new Teams auth config surface plus contributor-supplied external live proof; automation cannot supply split-tenant deployment evidence.

Security
Needs attention: The diff is security-sensitive because the new Graph tenant override must not bypass existing Teams cloud and China endpoint boundaries.

Review findings

  • [P1] Preserve Teams cloud options in Graph token loading — extensions/msteams/src/graph.ts:238
Review details

Best possible solution:

Rebase onto current main, thread graphTenantId through resolveGraphToken while preserving resolveMSTeamsSdkCloudOptions and the China guard, update config/docs/metadata/tests, then add redacted live split-tenant Teams/Graph proof before maintainer acceptance.

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

Do we have a high-confidence way to reproduce the issue? Partially: source inspection shows current main has no graphTenantId and always uses the Teams credentials tenant for Graph token setup, but no live split-tenant Teams/Graph deployment proof is available.

Is this the best way to solve the issue?

Is this the best way to solve the issue? Not yet. The Teams Graph-token path is the right owner, but the best fix must preserve current cloud/serviceUrl and China handling while proving the new tenant split in a credentialed setup.

Full review comments:

  • [P1] Preserve Teams cloud options in Graph token loading — extensions/msteams/src/graph.ts:238
    Current main rejects China Graph operations and passes resolveMSTeamsSdkCloudOptions(msteamsCfg) into loadMSTeamsSdkWithAuth. This branch's changed call uses loadMSTeamsSdkWithAuth(graphCreds) without those options, so resolving the conflict in this shape would drop sovereign-cloud routing and the China fail-closed guard.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded Microsoft Teams auth/config improvement with limited blast radius but real operator impact.
  • merge-risk: 🚨 compatibility: The PR adds a new documented Teams config key and generated channel metadata surface.
  • merge-risk: 🚨 auth-provider: The PR changes which Microsoft tenant is used for acquiring Teams Microsoft Graph tokens.
  • merge-risk: 🚨 security-boundary: The changed token path intersects sovereign-cloud routing and the current China Graph fail-closed boundary.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists tests and a coordinated provisioning PR, but no redacted live Teams/Graph split-tenant output, terminal log, screenshot, recording, or linked artifact showing the after-fix behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +10, Tests +32, Docs +5, Generated 0. Total +47 across 8 files.

View PR surface stats
Area Files Added Removed Net
Source 3 12 2 +10
Tests 2 32 0 +32
Docs 2 6 1 +5
Config 0 0 0 0
Generated 1 8 8 0
Other 0 0 0 0
Total 8 58 11 +47

Security concerns:

  • [medium] Graph token path can drop cloud boundary — extensions/msteams/src/graph.ts:238
    The changed call constructs the Graph-token SDK app without current main's cloud options or China fail-closed guard, which can misroute sovereign-cloud Graph token acquisition after conflict resolution.
    Confidence: 0.88

What I checked:

  • Repository policy read: Root and scoped AGENTS.md files were read; the review applied OpenClaw rules that config/auth/provider routing changes are compatibility-sensitive and docs/plugin boundaries must be checked. (AGENTS.md:1, 1052652a7168)
  • Live PR state: The PR is open, external, maintainer-modifiable, head c6dcc0f9e41f9ba44b949ef4883cd50dc04ae53d, and GitHub reports mergeable: CONFLICTING / mergeStateStatus: DIRTY; the body lists tests and a coordinated platform PR but no live split-tenant proof artifact. (c6dcc0f9e41f)
  • Current main Graph boundary: Current main rejects channels.msteams.cloud="China" for Graph operations and passes resolveMSTeamsSdkCloudOptions(msteamsCfg) into the SDK app used for Graph token acquisition. (extensions/msteams/src/graph.ts:216, 1052652a7168)
  • PR changed call site: The PR head creates graphCreds with the override tenant but calls loadMSTeamsSdkWithAuth(graphCreds) without the current-main cloud options, which is the concrete line-level blocker after rebase. (extensions/msteams/src/graph.ts:238, c6dcc0f9e41f)
  • Current docs contract: The Teams docs say channels.msteams.cloud selects Teams SDK authentication, JWT validation, token services, and Graph scope, and that China Graph helpers are disabled until Azure China Graph routing is implemented. Public docs: docs/channels/msteams.md. (docs/channels/msteams.md:679, 1052652a7168)
  • Dependency contract checked: Pinned @microsoft/[email protected] defines separate Graph scopes for Public, USGov, DoD, and China, and pinned @microsoft/[email protected] derives Graph base URL/token manager behavior from the configured App cloud.

Likely related people:

  • heyitsaamir: Auth and SDK history shows PR fix(msteams): rebase TeamsSDK patterns to simplify Teams Integration #76262 added the SDK migration, cloud.ts, and current Teams Graph/cloud behavior across the affected files. (role: introduced current Teams SDK/cloud boundary; confidence: high; commits: 04c29825356f; files: extensions/msteams/src/graph.ts, extensions/msteams/src/cloud.ts, extensions/msteams/src/sdk.ts)
  • steipete: Merged the Teams SDK migration and authored recent follow-up commits in that PR history for China cloud boundaries, service URL contract, and SDK proactive hardening. (role: recent area contributor and merger; confidence: high; commits: 04c29825356f, 2c61241c74c2, 9b54f5ab56de; files: extensions/msteams/src/graph.ts, extensions/msteams/src/cloud.ts, extensions/msteams/src/send-context.ts)
  • sudie-codes: Merged Graph/delegated-auth and group-management PRs added substantial Graph token, pagination, and Graph action behavior in the same area. (role: Teams Graph feature contributor; confidence: high; commits: 355794c24a39, f71ee71787c7; files: extensions/msteams/src/graph.ts, extensions/msteams/src/graph-messages.ts, extensions/msteams/src/token.ts)
  • HDYA: Federated credential support changed token.ts, sdk.ts, config schema/types, and docs adjacent to the proposed Graph tenant override. (role: Teams auth feature introducer; confidence: medium; commits: 26f633b604fd; files: extensions/msteams/src/token.ts, extensions/msteams/src/sdk.ts, src/config/types.msteams.ts)
  • eleqtrizit: Merged serviceUrl binding work defines current Teams auth/serviceUrl behavior that this PR must preserve during rebase. (role: adjacent Teams service URL contributor; confidence: medium; commits: 2c3d7f5badbe; files: extensions/msteams/src/sdk.ts, extensions/msteams/src/monitor.ts, extensions/msteams/src/attachments/bot-framework.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.

@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: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels May 27, 2026
@clawsweeper

clawsweeper Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@BradGroux BradGroux mentioned this pull request May 27, 2026
Draft
@BingqingLyu

This comment was marked as spam.

Sign in to view
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed 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. labels May 29, 2026
@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: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 15, 2026
This was referenced Jun 16, 2026
Closed
Closed
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. label Jun 21, 2026
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 21, 2026
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 14, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 14, 2026
@clawsweeper

clawsweeper Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: Support separate Teams Graph tenant This is item 1/1 in the current shard. Shard 30/31.

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.

@openclaw-barnacle

openclaw-barnacle Bot commented Jul 22, 2026

Copy link
Copy Markdown

Closing due to inactivity.
If you believe this PR should be revived, post in #clawtributors on Discord to talk to a maintainer.
That channel is the escape hatch for high-quality PRs that get auto-closed.

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 docs Improvements or additions to documentation gateway Gateway runtime merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DanBotero @BingqingLyu @RomneyDa