Skip to content

[3/5] Add account-bound Graph audience discovery - #271

Merged
SophieS0ng merged 2 commits into
users/rebova/org-announcements-prereleasefrom
users/rebova/org-announcements-review-audience
Sep 25, 2026
Merged

SophieS0ng merged 2 commits into
users/rebova/org-announcements-prereleasefrom
users/rebova/org-announcements-review-audience

Conversation

@rebova-microsoft

Copy link
Copy Markdown
Contributor

Description

Add read-only audience discovery using an independent Microsoft Graph resource token bound to the intended authoring tenant and account.

Current diff

  • Search display name OR mail in one initial Graph query, preserving escaping, stable deduplication and eligibility policy.
  • Keep the limits of 50 candidates per page, three pages, and 20 eligible results, with explicit exhaustion reporting.
  • Preserve bounded metadata caching and rejected-token invalidation. Log only predefined benign sign-in codes or a fixed unknown marker.

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-page on September 10, 2026. This stack remains pinned to cacb1bec056428809f1ccb0383561190d516bee4, which is an ancestor of merge commit 8a04f5f40f334e729a3497877edca655730f1be2. 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

  • Live Graph, authoring-service, hosted-widget, and end-to-end integration checks are deferred and remain required before rollout.
  • This is not release approval and performs no deployment. The future release branch is still TBD.
  • The prerelease integration branch remains baseline-only until reviewed chunks are merged through the normal review process.
  • Relevant offline tests and static checks pass.
  • Python 3.11 CI results.
  • Authorized live integration and end-to-end validation.

@rebova-microsoft rebova-microsoft left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread .github/workflows/ci.yml
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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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/oid checks guard identity consistency; they are not authorization. Tokens without readable matching claims are rejected. The services remain responsible for authorization and save validity.
  • search_groups combines 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_groups batches IDs and caches only successful metadata for five minutes, with a 512-entry cap. build_audience_metadata restores 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.

Comment thread tests/mcp/_mcp_modules.py
)


def load_org_announcements_directory_modules() -> dict[str, ModuleType]:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@rebova-microsoft

Copy link
Copy Markdown
Contributor Author

Merge commit 26c44f6 brings the updated contracts from PR #270 into this existing audience branch so the stack retains its ancestry. No audience implementation changes were added by this follow-up.

os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "agentconfig_core"),
)

from base_client import account_for_identity, validate_token_identity # noqa: E402

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This doesn’t directly reuse the FlightCheck  GraphClient  because that client is synchronous and CLI-oriented, including writing sign-in status to stdout, which isn’t safe for an async stdio MCP server. It does reuse the same Graph app ID, shared token cache, and tenant/account validation, so it doesn’t introduce another registration or separate sign-in state. The added code is the async, audience-specific search and metadata layer; we can factor the remaining common auth/HTTP plumbing into a shared helper later if needed.

@rebova-microsoft
rebova-microsoft force-pushed the users/rebova/org-announcements-review-audience branch from 26c44f6 to 252fc00 Compare September 18, 2026 22:20
Base automatically changed from users/rebova/org-announcements-review-contracts to users/rebova/org-announcements-prerelease September 24, 2026 21:47
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
@SophieS0ng
SophieS0ng force-pushed the users/rebova/org-announcements-review-audience branch from 252fc00 to 0ad13bf Compare September 24, 2026 21:47
@SophieS0ng
SophieS0ng marked this pull request as ready for review September 24, 2026 21:48
@apurvabanka

Copy link
Copy Markdown
Contributor

Reviewed graph_directory_client.py end-to-end and ran the new suite locally (123 passed). Solid work — the eligibility policy, the 401 compare-and-swap invalidation, and the "never render a raw GUID as a name" rule are all well reasoned and well tested. A few gaps, summarized first:

# Severity Location Gap
1 🟠 High resolve_groups (~L616–639) @odata.nextLink ignored → valid groups silently marked invalid
2 🟠 High resolve_groups (~L606) Fully-cached resolve still acquires a token; can trigger an interactive prompt
3 🟡 Medium search_groups (~L548–554) exhausted hard-coded False at the result cap
4 🟡 Medium _discard_credentials (~L384–389) Injected metadata_cache silently replaced after a 401
5 ⚪ Low is_eligible_group (~L159) Missing/null groupTypes makes a valid group permanently invalid
6 ⚪ Low _get (~L507–512) 429 marked retryable but Retry-After discarded, no backoff
7 ℹ️ Info — Module not yet imported anywhere (expected for 3/5)

1. 🟠 High — resolve_groups ignores @odata.nextLink
search_groups pages properly, but the batched $filter lookup reads only the first page. Graph can return a partial page plus a nextLink regardless of $top, and any dropped ID then falls through build_audience_metadata as "Unavailable group" / isValid: false. That's a silent false-negative on a perfectly valid audience, and no test covers it. Either follow nextLink the way search_groups does, or add a test that pins the single-page assumption.

2. 🟠 High — cached-only resolve still forces token acquisition
_ensure_client() is called before the cache check, so a resolve that's fully served from the TTL cache still acquires a token, which can fall through to acquire_token_interactive. Since every mutation refreshes manager state and re-resolves audiences, that's a surprise browser prompt in the middle of a save. Suggest moving _ensure_client() below the pending loop and returning early when pending is empty.

3. 🟡 Medium — exhausted is wrong in the exactly-exhausted case
When the 20-result cap is hit, the early return hard-codes exhausted: False even if that page carried no nextLink. The docstring says the flag distinguishes exhaustion from capping, so a caller rendering "more matches available" will be wrong here.

4. 🟡 Medium — _discard_credentials swaps out an injected cache
It replaces self._metadata_cache with a fresh internal instance, so a caller that passed in a shared metadata_cache silently loses it after the first 401. Clearing without replacing (or only replacing when the cache is self-owned) keeps the injection contract intact.

5. ⚪ Low — groupTypes must be a list
is_eligible_group returns False when groupTypes is missing, and the test at line 153 pins that. Safe today because $select requests it, but a null or omitted value turns a valid security group permanently invalid. Treating missing as [] loses no policy.

6. ⚪ Low — 429 advertises a retry that doesn't exist
retryable=True is set for 429/502/503/504 but Retry-After is discarded and there's no backoff. Worth either honoring the header or documenting that retry is the caller's job.

7. ℹ️ Info — nothing imports this module yet
Expected for 3/5, but it does mean the eligibility policy and error codes get no end-to-end exercise until the server tools land in 4/5.


Checked and fine: portalocker is covered transitively via msal-extensions[portalocker]; account_for_identity / validate_token_identity / create_token_cache all exist in agentconfig_core; the GRAPH_ACCESS_TOKEN override is tenant/oid-validated; the new test file is registered in CI.

@SophieS0ng SophieS0ng closed this Sep 25, 2026
@SophieS0ng SophieS0ng reopened this Sep 25, 2026
Follow Graph next links during audience resolution, report exact search exhaustion correctly, and cover cached token reuse with regression tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 19335911-b012-4b7f-b179-e4caeae7ea69
@SophieS0ng
SophieS0ng merged commit f122ad7 into users/rebova/org-announcements-prerelease Sep 25, 2026
7 checks passed
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.

5 participants