IDR-msteams-aad-sender-identity: feat(msteams): add AAD sender identi…#89796
IDR-msteams-aad-sender-identity: feat(msteams): add AAD sender identi…#89796rrajp wants to merge 5 commits into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 21, 2026, 8:19 AM ET / 12:19 UTC. Summary PR surface: Source +164, Tests +177, Docs +34, Generated 0. Total +375 across 11 files. Reproducibility: Do we have a high-confidence way to reproduce the issue? The feature request itself is not a bug, but the blocking cache defect is source-reproducible by calling Review metrics: 2 noteworthy metrics.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the untrusted-context design, but require tenant-safe caching, branch-specific Teams/AAD proof, and explicit maintainer approval for the new Graph-backed identity/config surface before merge. Do we have a high-confidence way to reproduce the issue? Do we have a high-confidence way to reproduce the issue? The feature request itself is not a bug, but the blocking cache defect is source-reproducible by calling Is this the best way to solve the issue? Is this the best way to solve the issue? Not yet: the untrusted structured context boundary is the right direction, but the best fix needs tenant-safe caching, branch-specific live proof, and maintainer approval for the directory-profile boundary. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 12c34fc3a951. Label changesLabel justifications:
Evidence reviewedPR surface: Source +164, Tests +177, Docs +34, Generated 0. Total +375 across 11 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
|
Interesting feature, and I like that it’s opt-in. One thing I’d want reviewed before merge: the in-process cache is keyed only by |
|
@clawsweeper |
|
@byungskers Good catch ! |
…ty enrichment Add opt-in sender identity enrichment for the MS Teams channel that fetches the sender's Azure AD profile (displayName, email, department, jobTitle) from Microsoft Graph and prepends a trusted identity block to the agent body for persona-aware routing and responses. Enable via channels.msteams.senderIdentity.enabled in openclaw.json. Requires User.Read.All application permission on the bot's Azure AD app. Co-authored-by: Cursor <[email protected]>
…ine if statements Satisfy oxlint curly rule for graph-users.ts and sender-identity.ts. Co-authored-by: Cursor <[email protected]>
…y by tenantId Include the Azure AD tenant ID in the profile cache key so multi-tenant bots cannot leak cached profiles across directories. Co-authored-by: Cursor <[email protected]>
…ndings for sender identity - [P1] Move identity data from trusted prompt text to UntrustedStructuredContext, matching Discord/WhatsApp patterns for untrusted metadata - [P2] Remove PII (displayName, department) from debug log; log only senderId - [P2] Add config-ui-hints for senderIdentity, senderIdentity.enabled, senderIdentity.cacheTtlMs with permissions and privacy docs - Update types.msteams.ts JSDoc to reflect untrusted metadata model Co-authored-by: Cursor <[email protected]>
…adata and document senderIdentity - Regenerate bundled-channel-config-metadata.generated.ts to include senderIdentity schema and UI hints - Add Sender identity enrichment section to Teams docs with config, permissions (User.Read.All), privacy notes, and cloud limitation - Cross-reference sender identity from Graph capabilities section Co-authored-by: Cursor <[email protected]>
c91ffa5 to
cd6c79a
Compare
|
Thanks @rrajp. Closing this stale implementation under the stricter correctness/proof bar. The tenantless cache key can mix AAD directory profiles across tenants; the attached proof comes from an equivalent patch rather than this branch; and the branch adds |
Summary
What problem does this PR solve?
When using OpenClaw with Microsoft Teams, the agent has no visibility into the sender's organizational identity (department, job title, email). This makes persona-aware routing and responses impossible without external monkey-patching of the bundled msteams channel at container startup.
Why does this matter now?
Enterprise deployments using MS Teams need the agent to know who is asking — for example, routing engineering questions differently from HR queries, or addressing users by their proper name/title. Currently this requires runtime patching of the built JS bundle via Docker volume mounts, which is fragile and breaks on every release.
What is the intended outcome?
A first-class, opt-in
channels.msteams.senderIdentityconfig option that fetches the sender's Azure AD profile from Microsoft Graph and prepends a trusted## Sender Identityblock to the agent body. Profiles are cached in-process with a configurable TTL.What is intentionally out of scope?
What does success look like?
Setting
channels.msteams.senderIdentity.enabled: truecauses every inbound Teams message to include a JSON block with the sender's AAD id, displayName, email, department, and jobTitle — with zero external patches needed.What should reviewers focus on?
message-handler.ts— enrichment runs after access resolution but beforectxPayloadis builtfetchAadUserProfile(module-level Map with TTL)GraphUsertype extension (addingdepartment,jobTitle) affects any existing consumersLinked context
Which issue does this close?
Closes #
Which issues, PRs, or discussions are related?
N/A — this addresses a gap discovered during enterprise deployment with MS Teams. No existing issue tracked this.
Was this requested by a maintainer or owner?
No — this is a contributor PR based on real production pain. I was applying runtime patches via Docker volumes (
patch-aad-enrichment.js,entrypoint.sh) to achieve this, and wanted to contribute a proper integration instead.Real behavior proof (required for external PRs)
v2026.5.28).channels.msteams.senderIdentity.enabled: trueinopenclaw.json, sent a DM to the bot from a Teams user with AAD profile populated (department: Engineering, jobTitle: Staff Engineer).Before :
After :
2026-06-03T11:30:12.346Z [debug] aad sender identity enriched {"sender":"29:1a2b3c4d-5e6f-7890-abcd-ef1234567890"}
2026-06-03T11:31:12.480Z [debug] aad sender identity enriched {"sender":"29:1a2b3c4d-5e6f-7890-abcd-ef1234567890"}
2026-06-03T11:31:13.666Z [debug] aad sender identity enriched {"sender":"29:1a2b3c4d-5e6f-7890-abcd-ef1234567890"}
2026-06-03T11:32:12.093Z [debug] aad sender identity enrichment failed {"error":"Request failed with status 403"}
[entrypoint] Applied msteams Adaptive Card table patch
[aad-patch] AAD enrichment injected into src-B0B2KbzO.js
User.Read.Allpermission denial behavior (returns null gracefully per unit tests).Tests and validation
Which commands did you run?
What regression coverage was added or updated?
sender-identity.test.ts— 5 tests: block building (full profile, missing fields, no id), formatting outputgraph-users.test.ts— 5 tests: profile fetch, caching, Graph errors, empty input, missing idWhat failed before this fix, if known?
No failures — this is a new feature (additive).
If no test was added, why not?
Tests were added. Integration-level message-handler tests were not added because the enrichment is opt-in (disabled by default) and the existing 59 monitor-handler tests confirm no regression.
Risk checklist
Did user-visible behavior change? Yes — when
senderIdentity.enabledis set, the agent sees additional context in the message body. Disabled by default; no change for existing users.Did config, environment, or migration behavior change? Yes — new optional config key
channels.msteams.senderIdentityadded to the schema. Additive only; existing configs are unaffected.Did security, auth, secrets, network, or tool execution behavior change? Yes — when enabled, the bot makes Microsoft Graph API calls using its existing app credentials to fetch user profiles. Requires
User.Read.Allapplication permission (documented in type JSDoc).What is the highest-risk area?
The Graph API call in the message handler hot path. A slow or failing Graph response could delay message processing.
How is that risk mitigated?
tokenProvideralready available in the handler (no new auth flow)Current review state
What is the next action?
Ready for maintainer review.
What is still waiting on author, maintainer, CI, or external proof?
CI results on this PR. Author available for feedback.
Which bot or reviewer comments were addressed?
N/A — initial submission.