Skip to content

feat(teams): wire Teams into AKConfig and harden the handler - #645

Open
SandunYL wants to merge 7 commits into
developfrom
fix/619-teams-integration
Open

feat(teams): wire Teams into AKConfig and harden the handler #645
SandunYL wants to merge 7 commits into
developfrom
fix/619-teams-integration

Conversation

@SandunYL

@SandunYL SandunYL commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Description

Makes the Microsoft Teams integration actually usable. AgentTeamsRequestHandler already read
Config.get().teams.*, but AKConfig had no teams section — so constructing the handler raised
AttributeError and the integration was effectively orphaned. This PR adds that config section,
hardens the handler around the Bot Framework delivery contract and attachment authorization, and
covers it with unit tests plus a live end-to-end test driven through the bot's Direct Line channel.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update
  • CI/CD update
  • Other (please describe):

Related Issues

Fixes #619

Changes Made

Configuration

  • _TeamsConfig added to AKConfigagent, agent_acknowledgement, app_id, app_password,
    tenant_id — so teams: YAML and AK_TEAMS__* env vars behave like every other integration.
  • tenant_id is documented as the tenant owning the bot's own app registration (the SDK's
    MicrosoftAppTenantId): required for a single-tenant registration, empty for a multi-tenant one.
    It is a different tenant from the one an app-only attachment download needs, which is read off the
    incoming activity.
  • aiohttp added to the teams extra.

Handler (teams_chat.py)

  • The agent now runs outside the webhook turn, via a proactive continue_conversation follow-up.
    A slow agent can no longer exceed the Bot Framework delivery timeout and make Azure redeliver the
    activity, which showed up as duplicate replies. Background tasks are strongly referenced so the
    loop cannot collect them mid-run.
  • HTTP semantics: a failed JWT returns 401 instead of 500 (Azure retries on 5xx, so an auth
    problem must not look like a server error), a malformed body returns 400, and invoke activities
    return the pipeline's InvokeResponse rather than a bare 200. An adapter-level on_turn_error
    fallback makes sure a failure anywhere still reaches the user.
  • MSAL clients are built lazily and cached per tenant — constructing one performs OIDC discovery
    over the network, which must not happen during process startup — and a client-credentials grant is
    never attempted against /common.
  • Attachment authorization is decided per host: pre-authenticated URLs (tempauth, access_token,
    authkey) are fetched with no header at all; Bot Connector hosts get a Bot Framework token;
    Graph and SharePoint hosts get an app-only token scoped to that specific host; an unrecognised
    host is fetched without a token rather than being handed one.
  • Size is enforced while streaming against api.max_file_size, and audio/video is rejected from
    activity metadata before anything is downloaded. Rejected, oversized, failed and unauthorized
    attachments each get their own message, since they need different remedies.
  • Mention stripping removes only the bot's own <at> tag, keeping other people's display names so
    the agent still sees who was referred to. Replies are chunked at 8000 characters, below the size
    at which Teams silently drops an activity.

Tests

  • ak-py/tests/test_teams_integration.py — 39 unit tests covering the turn lifecycle, auth
    responses, mention stripping, tenant resolution, per-host download authorization, size limits and
    reply chunking.
  • e2e/tests/test_teams.py — live round trip. Teams has no API that lets a user account message a
    bot, so the activity is sent through the same Azure Bot's Direct Line channel: it is signed and
    delivered by the real Bot Framework service, exercising JWT validation, the proactive follow-up and
    the connector reply.
  • examples/api/teams refreshed and added to the e2e matrix in .github/test-config.yaml.

e2e harness

  • Teams wired into e2e/app: API Gateway route, terraform variables/outputs, task env, config, and
    optional handler construction that degrades gracefully when credentials are absent.
  • e2e-messaging-deploy now builds ak-py from the checkout and installs that wheel over the
    released agentkernel the lock resolves. Without this the harness exercises the last PyPI release,
    so an integration added on a branch can never be tested before it ships.
  • New e2e_messaging_only dispatch input runs just the messaging harness, skipping the weekly cloud
    matrix.

Documentation

  • Teams guide and integration README updated: tenant semantics, the two-tenant distinction, and
    troubleshooting for the failure modes this work surfaced — including a webhook that answers 200
    while every reply dies silently, and what AADSTS7000229 vs AADSTS700016 each mean.
  • e2e/README.md documents the Teams setup, and that an app registration created with
    az ad app create or the Graph API needs az ad sp create for its service principal.

Testing

  • Unit tests pass locally
  • Integration tests pass locally
  • Manual testing completed
  • New tests added for changes

Verified live against the deployed e2e harness — run
32240584744:

test_teams.py::test_teams_round_trip PASSED
4 passed, 3 skipped

Getting there required an Azure-side fix worth recording: the bot's app registration existed with no
service principal, so no authority could issue it a token. The webhook answered 200 and every reply
failed silently, because inbound activities are validated against Bot Framework public keys and only
need to match the App ID, while the reply needs a token minted from the app password. az ad sp create --id <app-id> resolved it.

Manual verification from the Teams client is still outstanding — the tenant's app policy blocks
installing the custom app ("You do not have permission to use this app here"). The only path this
adds over Direct Line is attachment download from smba.trafficmanager.net/SharePoint hosts, which
Direct Line cannot reach; that path is covered by unit tests.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Additional Notes

Known follow-up: _bot_framework_token() builds MicrosoftAppCredentials(app_id, app_password)
without a tenant, so a single-tenant bot cannot download connector-hosted attachments (inline
pasted images) — the same AADSTS700016 class of failure the tenant work above fixed for outbound
replies. Direct Line does not exercise that path, so CI does not catch it. Passing
self._tenant_id or None as channel_auth_tenant is the fix.

@SandunYL
SandunYL marked this pull request as ready for review August 19, 2026 10:29
@SandunYL
SandunYL requested a review from amithad as a code owner August 19, 2026 10:29
@SandunYL SandunYL changed the title Fix/619 teams integration feat(teams): wire Teams into AKConfig and harden the handler Aug 19, 2026

@amithad amithad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review against Agent Kernel's architecture, code-quality, testing, and messaging-integration skills. Overall: solid work, no blockers found.

Assessment

  • The PR closes the #619 gap exactly along the house pattern: _TeamsConfig follows the _TelegramConfig idiom, the handler stays on the ChatService execution core with a prebuilt request list (the correct layer per the chat execution rubric), coupling direction is respected, and the ak-dev-new-messaging-integration checklist is now fully satisfied (config section, optional-dependency extra, example, tests, docs, chunking, webhook auth). Both the dev skill and the bundled ak-add-integration skill were updated to match.
  • Worth calling out as a real security improvement: the old code minted a Graph-scoped bearer and sent it to any non-SharePoint download host; the new per-host authorization (pre-authenticated URLs get no header, unknown hosts never get a token) removes that token-leak vector.
  • Test coverage is thorough: 39 unit tests using the object.__new__ handler pattern from the testing conventions, the refreshed example test, and a live Direct Line round trip. CI is fully green, including the new examples/api/teams e2e matrix entry.
  • No spec documents exist for #619 (none in the PR, none on develop), so the review is against the skills rubric only.

Findings: 5 suggestions, 2 nits, posted inline.

Notes that could not be anchored to diff lines

  • Root README.md line 183 (the ak-add-integration row in the skills table) still lists "Slack, WhatsApp, Messenger, Instagram, Telegram, Gmail" without Teams.
  • docs/docs/advanced/multimodal.md and examples/api/teams/config.yaml are full-file rewrites that are pure CRLF-to-LF normalization (content-identical). Fine as cleanup, just worth knowing when reading the diff.
  • Possible future hardening (pre-existing behavior, not introduced here): attachment contentUrl values come from the activity payload, and unrecognized hosts are still fetched, albeit without a token, with the response fed to the agent. A crafted Direct Line activity can point the bot at internal URLs. An allowlist of fetchable hosts would close that in a follow-up.

conversation_id = activity.conversation.id
text = self._strip_mentions(activity)
attachments = [a for a in (activity.attachments or []) if (a.content_type or "") != "text/html"]
user_name = activity.from_property.name if activity.from_property else "User"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] user_name becomes None when from_property exists but carries no name, producing user-visible "Hi None, ..." and "Sorry None, ..." messages.

  • Direct Line, the channel this PR's own e2e test drives, sends from with an id and no name, so the path is reachable today.
  • Fix: user_name = (activity.from_property.name if activity.from_property else None) or "User".

async def _bot_framework_token(self) -> Optional[str]:
"""Return the bot's own Bot Framework token, used for Bot Connector attachment URLs."""
if self._bot_credentials is None:
self._bot_credentials = MicrosoftAppCredentials(self._app_id, self._app_password)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Consider landing the known follow-up from the PR description here rather than deferring: pass channel_auth_tenant=self._tenant_id or None to MicrosoftAppCredentials.

  • Without it a single-tenant bot cannot download connector-hosted attachments (inline pasted images): the same AADSTS700016 failure class this PR fixes for outbound replies.
  • It is a one-line change, the test scaffolding (_bot_framework_token mocking) already exists, and CI can never catch the regression later since Direct Line does not exercise this path.

uv export --no-hashes --no-dev > requirements.txt
uv pip install -r requirements.txt --target=dist/data
rm -rf dist/data/agentkernel dist/data/agentkernel-*.dist-info
uv pip install --no-deps --target=dist/data ../../ak-py/dist/agentkernel-*.whl

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The --no-deps overlay means a branch that adds a new runtime dependency to an ak-py extra deploys without it.

  • requirements.txt is exported from e2e/app/uv.lock, which pins the released agentkernel's dependency set; the branch wheel is then dropped in with no dependency resolution.
  • This PR is itself the first example: aiohttp was added to the teams extra, and the deploy only works because the e2e app happens to depend on aiohttp directly.
  • Suggestion: after the lock-based install, install the wheel without --no-deps (already-satisfied pins are left alone, newly added deps get resolved), or export requirements from the branch checkout instead of the lock.

name = "agentkernel"
version = "0.8.1"
source = { registry = "https://pypi.org/simple" }
source = { editable = "../../../ak-py" }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The committed lock resolves agentkernel to an editable local path (../../../ak-py) while pyproject.toml still declares the registry dependency agentkernel[...]>=0.8.1.

  • Every other example lock (e.g. examples/api/telegram/uv.lock) records the registry source; this looks like a locally regenerated lock committed by accident.
  • The example is not standalone-usable in this state (the relative path only exists inside the repo checkout), and scripts/update_examples_version.py --force-lock expects registry pins.
  • Regenerate with uv lock against the registry. The published 0.8.1 lacking the teams: block is expected pre-release; the publish-time version bump resolves it, same as any other integration example.

|----------|--------|-------|-------|
| **Telegram** | ✅ | ✅ | Photos + documents |
| **REST API** | ✅ | ✅ | Via `AgentRequestImage` / `AgentRequestFile` |
| **CLI** | ❌ | ❌ | Text only |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This PR makes Teams a full image and file platform, but the Supported Integrations table still lists only Telegram, REST API, and CLI.

  • Add a Teams row (Images: yes, Files: yes; audio/video rejected, api.max_file_size enforced while streaming).
  • Slack/WhatsApp being absent is a pre-existing gap, fine to leave for a separate pass.

Comment thread e2e/tests/test_teams.py
"Error sending agent response.",
"The agent returned an empty response.",
"Sorry, an error occurred while processing your request.",
"Sorry Alice, an error occurred while processing your request.",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] "Sorry Alice, ..." is unreachable here: "Alice" is the unit-test fixture name, and the Direct Line user carries no display name, so the deployed handler would currently say "Sorry None, ..." ("Sorry User, ..." once the user_name fallback in teams_chat.py is fixed). Replace it with the reachable variant so the error-fallback detection actually fires.

* **`AADSTS700016`** — "application ... not found in the directory". The token was requested from
the wrong tenant. If the directory in the message is `d6d49420-f39b-4df7-a1dc-d59a935871db`, that
is the Bot Framework tenant, meaning `teams.tenant_id` was left empty for a single-tenant
registration. See "Tenant ID" below.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] "See "Tenant ID" below": the Tenant ID section (line 91, under Features) is above this point, so this should read "above".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TASK] Investigate and fix breaking points of teams integration in Agent Kernel

2 participants