1. extensions/msteams/src/thread-parent-context.ts, line 86-88 (link)

   Error results no longer cached; persistent 403s will retry on every message

   Before this PR, fetchChannelMessage silently caught errors and returned undefined. fetchParentMessageCached cached that undefined, so repeated calls for the same parent (e.g., a burst of replies) would not re-hit Graph. Now that fetchChannelMessage propagates errors, the await fetchParent(...) on line 86 throws instead of returning undefined, and touchLru is never reached — meaning nothing is cached on failure.

   In environments where Graph is persistently inaccessible (wrong permissions, restricted tenant), every incoming channel thread message will trigger two failing Graph calls (parent + replies) without any backoff or negative-result cache. The test "caches undefined (Graph error) so failures do not re-fetch on burst" in thread-parent-context.test.ts still passes because it injects a mock fetcher that returns undefined — it no longer describes the real behavior of the default fetchChannelMessage.

   Consider wrapping the await fetchParent(...) in a try/catch that stores a sentinel on error so burst retries still hit the cache:

   let message: GraphThreadMessage | undefined;
   try {
     message = await fetchParent(token, groupId, channelId, parentId);
   } catch {
     // Cache the failure so burst retries do not hammer Graph on persistent errors.
     touchLru(parentCache, key, { message: undefined, expiresAt: now + PARENT_CACHE_TTL_MS }, PARENT_CACHE_MAX);
     throw;
   }
   touchLru(parentCache, key, { message, expiresAt: now + PARENT_CACHE_TTL_MS }, PARENT_CACHE_MAX);
   return message;

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: extensions/msteams/src/thread-parent-context.ts
   Line: 86-88
   
   Comment:
   **Error results no longer cached; persistent 403s will retry on every message**
   
   Before this PR, `fetchChannelMessage` silently caught errors and returned `undefined`. `fetchParentMessageCached` cached that `undefined`, so repeated calls for the same parent (e.g., a burst of replies) would not re-hit Graph. Now that `fetchChannelMessage` propagates errors, the `await fetchParent(...)` on line 86 throws instead of returning `undefined`, and `touchLru` is never reached — meaning nothing is cached on failure.
   
   In environments where Graph is persistently inaccessible (wrong permissions, restricted tenant), every incoming channel thread message will trigger two failing Graph calls (parent + replies) without any backoff or negative-result cache. The test `"caches undefined (Graph error) so failures do not re-fetch on burst"` in `thread-parent-context.test.ts` still passes because it injects a mock fetcher that returns `undefined` — it no longer describes the real behavior of the default `fetchChannelMessage`.
   
   Consider wrapping the `await fetchParent(...)` in a try/catch that stores a sentinel on error so burst retries still hit the cache:
   
   ```ts
   let message: GraphThreadMessage | undefined;
   try {
     message = await fetchParent(token, groupId, channelId, parentId);
   } catch {
     // Cache the failure so burst retries do not hammer Graph on persistent errors.
     touchLru(parentCache, key, { message: undefined, expiresAt: now + PARENT_CACHE_TTL_MS }, PARENT_CACHE_MAX);
     throw;
   }
   touchLru(parentCache, key, { message, expiresAt: now + PARENT_CACHE_TTL_MS }, PARENT_CACHE_MAX);
   return message;
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

2. extensions/msteams/src/graph.ts, line 217-229 (link)

   graphTenantId path untested; new App instance created on every call

   loadMSTeamsSdkWithAuth(graphCreds) is called on every invocation of resolveGraphToken. For the cross-tenant path (graphTenantId !== tenantId), this instantiates a second Bot Framework App per call. For CLI commands that call resolveGraphToken in a loop this can be expensive.

   Additionally, the new graphCreds substitution has no test coverage in graph.test.ts — neither the token acquisition with graphTenantId nor the fallback when graphTenantId === tenantId. Similarly, token.test.ts has no cases that exercise graphTenantId propagation through resolveMSTeamsCredentials. For a feature that changes which OAuth tenant is used to acquire tokens, adding at least happy-path tests would help prevent regressions.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: extensions/msteams/src/graph.ts
   Line: 217-229
   
   Comment:
   **`graphTenantId` path untested; new `App` instance created on every call**
   
   `loadMSTeamsSdkWithAuth(graphCreds)` is called on every invocation of `resolveGraphToken`. For the cross-tenant path (`graphTenantId !== tenantId`), this instantiates a second Bot Framework `App` per call. For CLI commands that call `resolveGraphToken` in a loop this can be expensive.
   
   Additionally, the new `graphCreds` substitution has no test coverage in `graph.test.ts` — neither the token acquisition with `graphTenantId` nor the fallback when `graphTenantId === tenantId`. Similarly, `token.test.ts` has no cases that exercise `graphTenantId` propagation through `resolveMSTeamsCredentials`. For a feature that changes which OAuth tenant is used to acquire tokens, adding at least happy-path tests would help prevent regressions.
   
   How can I resolve this? If you propose a fix, please make it concise.

Best possible solution:

Land a narrow Teams plugin change that keeps Bot Framework reply tokens on the bot tenant, uses graphTenantId only for Graph app-only token acquisition, avoids raw secret retention in cache keys, and keeps config schema, docs, generated metadata, tests, and changelog attribution aligned.

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

Yes for the static behavior: current main has no graphTenantId config/env surface and mints Graph tokens through the same Teams app provider used for Bot Framework replies. A live reproduction would require a split Azure/M365 tenant setup, which was not available in this read-only review.

Is this the best way to solve the issue?

No, not merge-ready as written. The split-provider design is the narrow right direction, but the cache key should not contain a raw client secret and the changelog needs the required attribution before merge.

Full review comments:

• [P2] Avoid caching raw Teams client secrets — extensions/msteams/src/graph.ts:49
buildGraphTokenProviderCacheKey stringifies creds.appPassword into the module-level cache key. That keeps a plaintext Azure client secret in memory beyond immediate credential resolution; use a non-secret discriminator or one-way fingerprint instead.
  Confidence: 0.84
• [P3] Add required changelog attribution — CHANGELOG.md:13
  The new user-facing Microsoft Teams changelog entry does not include the repo-required Thanks @... credit. Add the contributor or reporter attribution to the same single-line entry before merge.
  Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.84

Security concerns:

• [low] Raw client secret retained in cache key — extensions/msteams/src/graph.ts:49
  The Graph token-provider cache key includes creds.appPassword, so a plaintext Azure client secret is stored in the module-level cache key for the cache lifetime. Use a non-secret key component or a one-way fingerprint.
  Confidence: 0.84

Acceptance criteria:

• pnpm test extensions/msteams/src/token.test.ts extensions/msteams/src/graph.test.ts extensions/msteams/src/graph-thread.test.ts extensions/msteams/src/thread-parent-context.test.ts
• pnpm exec oxfmt --check --threads=1 extensions/msteams/src/graph.ts extensions/msteams/src/token.ts extensions/msteams/src/monitor.ts extensions/msteams/src/monitor-handler/message-handler.ts extensions/msteams/src/thread-parent-context.ts docs/channels/msteams.md src/config/types.msteams.ts src/config/zod-schema.providers-core.ts
• pnpm config:docs:gen/check
• pnpm check:changed

What I checked:

• Current main lacks requested surface: Exact search found no graphTenantId, MSTEAMS_GRAPH_TENANT_ID, graphTokenProvider, or effectiveGraphTokenProvider symbols on current main, while related Teams Graph helpers are present. (a256745323ab)
• Current config type has no Graph tenant override: MSTeamsConfig defines tenantId and then authType; no Graph-specific tenant field exists on current main. (src/config/types.msteams.ts:95, a256745323ab)
• Current schema rejects graphTenantId: The Teams Zod schema accepts tenantId and then authType; graphTenantId is absent from validation. (src/config/zod-schema.providers-core.ts:1571, a256745323ab)
• Current Graph token path uses the normal Teams credentials: resolveGraphToken loads the Teams SDK with creds directly and creates a token provider from that same app before requesting the Graph token. (extensions/msteams/src/graph.ts:213, a256745323ab)
• Current thread context uses Bot Framework token provider: Thread context fetching calls tokenProvider.getAccessToken("https://graph.microsoft.com") with no separate Graph provider dependency. (extensions/msteams/src/monitor-handler/message-handler.ts:620, a256745323ab)
• PR implements the requested split-tenant path: The latest PR diff adds config/docs/env metadata, carries graphTenantId through secret and federated credentials, builds separate Graph token providers, and wires the handler to use graphTokenProvider when present. (extensions/msteams/src/monitor.ts:239, a07328fe0191)

Likely related people:

• steipete: Recent remote history shows multiple May/April 2026 refactors touching the Teams Graph/token and message-handler paths, and local blame for the current checked-out Teams files points to recent maintainer commits. (role: recent maintainer; confidence: high; commits: 3f002b10d281, ffe67e9cdc9e, 02ebac6250bf; files: extensions/msteams/src/graph.ts, extensions/msteams/src/token.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
• HDYA: Introduced Microsoft Teams federated credential support, which is directly adjacent to the PR's secret/federated graphTenantId credential routing. (role: introduced related auth behavior; confidence: medium; commits: 26f633b604fd; files: extensions/msteams/src/token.ts, extensions/msteams/src/graph.ts, src/config/types.msteams.ts)
• sudie-codes: Recent Teams Graph action, delegated auth, pagination, and group-management commits changed the Graph token/helper area this PR extends. (role: Graph and delegated-auth contributor; confidence: medium; commits: 355794c24a39, f71ee71787c7, 0f192710924f; files: extensions/msteams/src/graph.ts, extensions/msteams/src/token.ts, extensions/msteams/src/monitor-handler/message-handler.ts)
• BradGroux: Auth/config history and the related Microsoft tracker show Brad Groux repeatedly involved in Teams and Microsoft ecosystem routing, including co-authorship on federated/Graph changes and direct Teams config commits. (role: adjacent Microsoft Teams maintainer; confidence: medium; commits: 26f633b604fd, 355794c24a39, fce81fccd859; files: src/config/types.msteams.ts, extensions/msteams/src/graph.ts, extensions/msteams/src/token.ts)

Remaining risk / open question:

• The latest PR diff still retains appPassword in the Graph token-provider cache key, which keeps a plaintext client secret in memory longer than necessary.
• The user-facing changelog entry is missing the repo-required non-bot contributor attribution.
• No live Azure split-tenant Teams environment was exercised during this read-only review; the need and fix direction are supported by static code paths and the PR author's manual verification claim.

Codex review notes: model gpt-5.5, reasoning high; reviewed against a256745323ab.