[3/5] Add account-bound Graph audience discovery - #271
rebova-microsoft wants to merge 1 commit into
Conversation
rebova-microsoft
left a comment
rebova-microsoft
left a comment
There was a problem hiding this comment.
File-by-file walkthrough of audience-group discovery, display metadata, and supporting tests. This is an explanatory review only, not code approval or resolution of existing feedback. This PR adds a directory client; selecting the target ESS agent remains separate, and MCP announcement runtime wiring is not added here.
| python -m pytest | ||
| tests/mcp/agentconfig_org_announcements/test_authoring_client.py | ||
| tests/mcp/agentconfig_org_announcements/test_drafts.py | ||
| tests/mcp/agentconfig_org_announcements/test_graph_directory.py |
There was a problem hiding this comment.
Walkthrough — Add directory-client coverage to the existing job.
This one-line change adds test_graph_directory.py to the Org Announcements test command introduced in the previous PR. The job now selects directory behavior alongside authoring requests and draft models, using the same Python setup and dependencies. It extends the intended automated checks; it neither performs a live Graph lookup nor establishes that the tests have passed.
| self._entries.clear() | ||
|
|
||
|
|
||
| class GraphDirectoryClient: |
There was a problem hiding this comment.
Walkthrough — Find audience groups and turn stored IDs into readable labels.
This client discovers announcement audiences, not the target ESS agent; shared core agent list/search handles that separate choice.
- Authentication uses a separate Graph resource token for the same authoring tenant and account, reusing the shared cache. Acquisition is lazy and runs off the async event loop. Local
tid/oidchecks guard identity consistency; they are not authorization. Tokens without readable matching claims are rejected. The services remain responsible for authorization and save validity. search_groupscombines display-name and email search in one initial request, then follows pages within bounds: at most 20 eligible results or three pages. It keeps first-seen order, removes duplicate results, and distinguishes exhausted results from a capped search. Eligibility includes non-dynamic security groups, mail-enabled security groups, and classic distribution lists; Unified and dynamic-membership groups are excluded.resolve_groupsbatches IDs and caches only successful metadata for five minutes, with a 512-entry cap.build_audience_metadatarestores each saved audience's order and repeated entries. Unresolved or unsupported IDs stay present as invalid entries labeled “Unavailable group,” rather than disappearing.- A 401 clears the rejected credentials and metadata cache so a later call can acquire credentials again; the failed request is not automatically replayed.
This supplies directory data and error handling for later wiring, not UI controls, announcement persistence, or a running MCP announcement server.
| ) | ||
|
|
||
|
|
||
| def load_org_announcements_directory_modules() -> dict[str, ModuleType]: |
There was a problem hiding this comment.
Walkthrough — Extend isolated loading for directory tests.
The new directory-specific helper loads client, drafts, and graph_directory_client through the existing feature-isolated importer. This keeps the directory tests aligned with the same announcement model/client objects while avoiding clashes with similarly named modules in other feature folders. It deliberately stops short of loading telemetry or the MCP server, so this PR can exercise its directory layer without depending on later runtime wiring.
| ("not-a-dict", False, "non-object"), | ||
| ], | ||
| ) | ||
| def test_eligibility_predicate(group, eligible, reason) -> None: |
There was a problem hiding this comment.
Walkthrough — Check audience lookup rules and recovery with controlled responses.
The tests supply fake groups, token claims, authentication results, and HTTP responses. Search cases exercise one combined name/email query, eligible group types, escaping, paging bounds, and duplicate removal. Metadata cases check expiry, bounded positive caching, readable fallback labels, and preservation of stored order and repeated IDs. Authentication cases check lazy acquisition, matching tenant/account selection, rejection of opaque or missing-account tokens, and recovery on a later call after a 401 without replaying the failed request. These are local behavior checks, not live Graph consent or authorization verification.
| os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "agentconfig_core"), | ||
| ) | ||
|
|
||
| from base_client import account_for_identity, validate_token_identity # noqa: E402 |
There was a problem hiding this comment.
Does this use the existing graph client from things like graphql? Want to make sure we don't have anything duplicative here, this is a lot of infra to be spun up on what should be existing functionality
Search eligible directory groups by name or email with one combined bounded Graph query. Bind independent Graph credentials and metadata caches to the intended authoring tenant and account, preserving safe invalidation and diagnostic handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eddd3818-bb74-42d3-bcf3-7e0670a57f27
26c44f6 to
252fc00
Compare
3 participants
Description
Add read-only audience discovery using an independent Microsoft Graph resource token bound to the intended authoring tenant and account.
Current diff
Feature contract
Supported delegated credentials must establish the same tenant/account context. Missing or incompatible token claims fail closed; no tenant-only relaxation, borrowed resource token, or identity-probing workaround is added.
Stack and dependency
This is slice 3/5. Head:
users/rebova/org-announcements-review-audience. Base:users/rebova/org-announcements-review-contracts. The current diff is only this slice against its immediate predecessor; these are stacked review chunks, not parallel PRs against the integration branch.Bootstrap prerequisite: #262 merged into
release/planner-landing-pageon September 10, 2026. This stack remains pinned tocacb1bec056428809f1ccb0383561190d516bee4, which is an ancestor of merge commit8a04f5f40f334e729a3497877edca655730f1be2. Unchanged prerequisite work is excluded from this slice. No rewrite is needed solely to account for that merge. The future release target remains TBD and its final promotion baseline must be confirmed separately.Testing
166 Python tests passed from the clean committed slice, covering Graph lookup, escaping, eligibility, identity matching, cache lifetime, and authentication diagnostics. Ruff and syntax checks passed; the MCP runtime was not yet present.
Local execution used Python 3.13.15. Python 3.11 GitHub Actions validation is pending; no local 3.11 pass is claimed.
Readiness