fix(teams): identify self-authored messages by UPN, not display name - #98
Merged
Conversation
Cursor-replay incident 2026-07-09: renaming the agent in the Entra directory silently no-op'd the filter_human_messages predicate, which compared displayName against a hard-coded string. Every outbound the agent had sent since the rename became re-eligible to be pushed as inbound; on 61 of 62 watched chats the cursor sat just before a self-authored pre-rename message, so a normal poll pass replayed those as fresh channel notifications. Fix identifies agents by UPN (config canonical, matched case-insensitively against message.from.user.userPrincipalName), with AAD object-id fallback where Graph does not surface UPN. Display name is never used for identity in the poll or send paths. Changes: - Adds ENTRABOT_AGENT_UPN as the canonical env; keeps ENTRABOT_AGENT_USER_UPN accepted for backwards-compatibility. - read() extracts sender_upn alongside sender_id. - filter_human_messages rewritten to match UPN-first, object-id fallback. Four caller sites in mcp_server.py updated (background poll, send_teams_message auto-wait, watch_teams_replies, wait_for_sponsor_dm). - whoami surfaces agent_upn and agent_object_id for observability. - Adds scripts/migrate_cursors_to_upn.py — idempotent one-shot that bumps last_ts and populates seen_ids_tail with recent self-authored message ids, suppressing the in-flight replay flood on next poll. Cursor blob keys do not include agent identity (verified via chat_cursors.cursor_key), so no key rename needed. - Adds Learning #69 and the "AGENT NAMES CHANGE - USE UPN" Non-Negotiable to CLAUDE.md and AGENTS.md. - 14 new tests, including a regression guard asserting that a message with matching displayName but non-matching UPN/object-id is NOT filtered out. Tested live 2026-07-09: migration ran cleanly, restart brought cursors_stale from 61/62 to 0/62; no self-authored replay observed post-restart across 62 watched chats. Full design: docs/architecture/PLAN-agent-identity-by-upn.md Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a Teams cursor-replay incident by changing “self-authored message” detection from mutable displayName matching to stable identity fields (UPN first, AAD object-id fallback), and adds a one-shot cursor migration script to suppress in-flight replay on next poll.
Changes:
- Update Teams message read + filtering so self-authored detection uses
sender_upn(case-insensitive) withsender_idfallback; remove displayName as an identity predicate. - Plumb agent UPN/object-id from config through all poll/watch/wait call sites and surface them via
whoamifor observability. - Add a one-shot migration script + tests + docs/runbook updates capturing the learning and operational steps.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
TODOS.md |
Records the incident, fix scope, migration, and test deltas. |
tests/tools/test_watch.py |
Expands filter tests to cover UPN-first, object-id fallback, and “displayName never used” regression. |
tests/test_mcp_server_integration.py |
Updates delegated-mode filtering tests to use the new filter signature + sender identity fields. |
tests/test_mcp_server_chat_cursors.py |
Updates cursor poll tests for new _poll_watched_chat signature + filter call shape. |
tests/test_cursor_migration.py |
Adds coverage for the new cursor migration script (dry-run, verify, idempotency, scoping). |
tests/smoke/smokeit.py |
Updates smoke polling to source agent identity from config and filter by UPN/object-id. |
src/entrabot/tools/teams.py |
Emits sender_upn from Graph and rewrites filter_human_messages to match by UPN/object-id (never displayName). |
src/entrabot/tools/identity.py |
Extends whoami output with agent_upn and agent_object_id for debugging/observability. |
src/entrabot/mcp_server.py |
Replaces hard-coded displayName filtering with config-sourced UPN/object-id across poll/watch/wait paths. |
src/entrabot/config.py |
Adds ENTRABOT_AGENT_UPN env support (with legacy alias fallback). |
scripts/migrate_cursors_to_upn.py |
Introduces one-shot cursor bump + seen_ids_tail seeding script for replay suppression. |
docs/runbooks/hard-won-learnings.md |
Adds Learning #69 documenting the failure mode and prevention rule. |
docs/engineering-status.md |
Updates status date/test count and notes the UPN-based fix as recent work. |
docs/architecture/PLAN-agent-identity-by-upn.md |
Adds the design/plan document describing the rationale, scope, and rollback. |
CLAUDE.md |
Adds a Non-Negotiable: don’t use displayName for identity; use UPN/object-id. |
AGENTS.md |
Mirrors the new Non-Negotiable for agent contributors. |
.env.example |
Documents ENTRABOT_AGENT_UPN (and legacy alias) for configuration. |
Comment on lines
+227
to
+243
| if _is_already_migrated(cursor.get("last_ts"), threshold): | ||
| summary["skipped_already_migrated"] += 1 | ||
| summary["chats"].append( | ||
| {"chat_id": chat_id, "action": "skip-already-migrated"} | ||
| ) | ||
| continue | ||
|
|
||
| self_ids = recent_self_authored_ids(chat_id, resolved_upn, resolved_oid) | ||
| merged_tail = bound_seen_ids( | ||
| list(cursor.get("seen_ids_tail") or []) + self_ids | ||
| ) | ||
| new_cursor = { | ||
| "last_ts": _bumped_ts(), | ||
| "seen_ids_tail": merged_tail[-MAX_SEEN_IDS_TAIL:], | ||
| "bootstrapped": True, | ||
| "last_written_at": _now_iso(), | ||
| } |
Comment on lines
+309
to
+312
| if _is_already_migrated(cursor.get("last_ts"), threshold): | ||
| migrated += 1 | ||
| else: | ||
| pending += 1 |
| assert result[0]["message_id"] == "m2" | ||
|
|
||
| def test_filter_upn_match_is_case_insensitive(self) -> None: | ||
| """UPNs are case-insensitive per RFC 822 local-part convention.""" |
…note Addresses Copilot review on PR #98: 1. Migration idempotency was time-based (last_ts >= now). That made accidental re-runs after the bump window keep advancing cursors forward, potentially skipping legitimate inbound that arrived while the MCP was down. Replaces the timestamp predicate with a stable migration flag blob at chat_cursors/_migrated_upn_fix.json. Presence of the flag is the sole skip condition; --dry-run never writes it. Deviation from the reviewer's exact suggestion (per-cursor marker): save_cursor's payload builder explicitly enumerates its four fields and strips anything else on the next poll write, so a per-cursor marker wouldn't survive one poll cycle. A separate namespace under the cursor prefix survives cursor updates and is robust to future save_cursor schema changes. 2. verify() no longer classifies "migrated" by last_ts drift. It now reports {flag_present, flag_written_at, flag_cursors_migrated, cursors_present} — stable across time. 3. test_filter_upn_match_is_case_insensitive docstring incorrectly claimed RFC 822 mandates case-insensitive local-parts. RFC 5321/5322 don't; the invariant we actually rely on is that Entra ID / Graph treats UPN comparisons as case-insensitive. Reworded. Tests: 1576 -> 1578 (+2 flag-based verify tests, replaced 1 timestamp verify test with 2 new ones covering flag-absent + flag-present states, plus a rerun-skip regression guard). Full suite green; ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
filter_human_messagescomparedmessage.from.user.displayNameagainst a hard-coded"EntraBot Agent"string. The rename to"EntraClaw Agent"silently no-op'd the filter; every outbound since the rename became eligible to be pushed back as inbound.ENTRABOT_AGENT_UPN, matched case-insensitively againstuserPrincipalName) with AAD object-id fallback.displayNameis never used for identity in the poll or send paths.CLAUDE.md+AGENTS.md.Full design in
docs/architecture/PLAN-agent-identity-by-upn.md.Test plan
displayName-only match is NOT filtered.ruff check .clean.scripts/migrate_cursors_to_upn.py --dry-runproduced expected plan; live run migrated all 62 cursors; MCP restart broughtcursors_stalefrom 61/62 → 0/62; no self-authored replay observed post-restart during ~30 min of interactive use.ENTRABOT_AGENT_UPNis documented in.env.exampleand picked up correctly on fleet nodes (verified locally; reviewer to sanity-check).🤖 Generated with Claude Code