feat(teams): wire Teams into AKConfig and harden the handler - #645
feat(teams): wire Teams into AKConfig and harden the handler #645SandunYL wants to merge 7 commits into
Conversation
amithad
left a comment
There was a problem hiding this comment.
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:
_TeamsConfigfollows the_TelegramConfigidiom, 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 theak-dev-new-messaging-integrationchecklist is now fully satisfied (config section, optional-dependency extra, example, tests, docs, chunking, webhook auth). Both the dev skill and the bundledak-add-integrationskill 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 newexamples/api/teamse2e 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.mdline 183 (theak-add-integrationrow in the skills table) still lists "Slack, WhatsApp, Messenger, Instagram, Telegram, Gmail" without Teams. docs/docs/advanced/multimodal.mdandexamples/api/teams/config.yamlare 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
contentUrlvalues 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" |
There was a problem hiding this comment.
[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
fromwith anidand noname, 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) |
There was a problem hiding this comment.
[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
AADSTS700016failure class this PR fixes for outbound replies. - It is a one-line change, the test scaffolding (
_bot_framework_tokenmocking) 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 |
There was a problem hiding this comment.
[suggestion] The --no-deps overlay means a branch that adds a new runtime dependency to an ak-py extra deploys without it.
requirements.txtis exported frome2e/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:
aiohttpwas added to theteamsextra, and the deploy only works because the e2e app happens to depend onaiohttpdirectly. - 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" } |
There was a problem hiding this comment.
[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-lockexpects registry pins. - Regenerate with
uv lockagainst the registry. The published 0.8.1 lacking theteams: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 | |
There was a problem hiding this comment.
[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_sizeenforced while streaming). - Slack/WhatsApp being absent is a pre-existing gap, fine to leave for a separate pass.
| "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.", |
There was a problem hiding this comment.
[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. |
There was a problem hiding this comment.
[nit] "See "Tenant ID" below": the Tenant ID section (line 91, under Features) is above this point, so this should read "above".
Description
Makes the Microsoft Teams integration actually usable.
AgentTeamsRequestHandleralready readConfig.get().teams.*, butAKConfighad noteamssection — so constructing the handler raisedAttributeErrorand 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
Related Issues
Fixes #619
Changes Made
Configuration
_TeamsConfigadded toAKConfig—agent,agent_acknowledgement,app_id,app_password,tenant_id— soteams:YAML andAK_TEAMS__*env vars behave like every other integration.tenant_idis documented as the tenant owning the bot's own app registration (the SDK'sMicrosoftAppTenantId): 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.
aiohttpadded to theteamsextra.Handler (
teams_chat.py)continue_conversationfollow-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.
problem must not look like a server error), a malformed body returns 400, and
invokeactivitiesreturn the pipeline's
InvokeResponserather than a bare 200. An adapter-levelon_turn_errorfallback makes sure a failure anywhere still reaches the user.
over the network, which must not happen during process startup — and a client-credentials grant is
never attempted against
/common.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.
api.max_file_size, and audio/video is rejected fromactivity metadata before anything is downloaded. Rejected, oversized, failed and unauthorized
attachments each get their own message, since they need different remedies.
<at>tag, keeping other people's display names sothe 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, authresponses, 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 abot, 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/teamsrefreshed and added to the e2e matrix in.github/test-config.yaml.e2e harness
e2e/app: API Gateway route, terraform variables/outputs, task env, config, andoptional handler construction that degrades gracefully when credentials are absent.
e2e-messaging-deploynow buildsak-pyfrom the checkout and installs that wheel over thereleased
agentkernelthe 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.
e2e_messaging_onlydispatch input runs just the messaging harness, skipping the weekly cloudmatrix.
Documentation
troubleshooting for the failure modes this work surfaced — including a webhook that answers 200
while every reply dies silently, and what
AADSTS7000229vsAADSTS700016each mean.e2e/README.mddocuments the Teams setup, and that an app registration created withaz ad app createor the Graph API needsaz ad sp createfor its service principal.Testing
Verified live against the deployed e2e harness — run
32240584744:
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, whichDirect Line cannot reach; that path is covered by unit tests.
Checklist
Additional Notes
Known follow-up:
_bot_framework_token()buildsMicrosoftAppCredentials(app_id, app_password)without a tenant, so a single-tenant bot cannot download connector-hosted attachments (inline
pasted images) — the same
AADSTS700016class of failure the tenant work above fixed for outboundreplies. Direct Line does not exercise that path, so CI does not catch it. Passing
self._tenant_id or Noneaschannel_auth_tenantis the fix.