From e183ff945ba76fea7096adb6916090a08af52e4b Mon Sep 17 00:00:00 2001 From: Reilly Bova Date: Fri, 11 Sep 2026 00:28:59 -0700 Subject: [PATCH 1/8] feat(announcements): expose scoped MCP authoring runtime Expose separate announcement manager and editor entry points with validated flat arguments, scoped retry context, captured directory leases, and widget-owned mutations. Preserve indeterminate and committed-refresh recovery, safe telemetry, and independent provider discovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eddd3818-bb74-42d3-bcf3-7e0670a57f27 --- .github/workflows/ci.yml | 5 +- .../agentconfig_org_announcements/server.py | 1286 ++++++++++ .../telemetry.py | 156 ++ .../agentconfig_core/test_agent_discovery.py | 19 +- .../test_authoring_client_lifecycle.py | 433 ++++ .../test_mcp_app_protocol.py | 2131 +++++++++++++++++ .../test_telemetry_privacy.py | 474 ++++ tests/mcp/test_import_isolation.py | 133 + 8 files changed, 4629 insertions(+), 8 deletions(-) create mode 100644 solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py create mode 100644 solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py create mode 100644 tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py create mode 100644 tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py create mode 100644 tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py create mode 100644 tests/mcp/test_import_isolation.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 784cbcf80..232b1abdf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -106,6 +106,7 @@ jobs: run: >- python -m pytest tests/mcp/agentconfig_core + tests/mcp/test_import_isolation.py tests/scripts/test_mcp_config.py tests/scripts/test_auth.py -q @@ -163,9 +164,7 @@ jobs: - name: Run Org Announcements tests run: >- 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 + tests/mcp/agentconfig_org_announcements -q flightcheck-tests: diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py new file mode 100644 index 000000000..00957de7d --- /dev/null +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -0,0 +1,1286 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""ESS Org Announcements MCP server. + +Announcements are scoped to the authenticated tenant and deployed ESS titleId. +The tenant comes exclusively from the AgentConfiguration access token; each +request carries its agent identity through every read, write, and refresh. + +Tool visibility is deliberate: + +* ``open_org_announcements`` is read-only and model/app-visible. It is the single + entry point a maker turn may use, and the widget uses it for scoped navigation. +* ``save_bulletin``, ``transition_bulletin``, and ``duplicate_bulletin`` are + app-visible only. Once the widget is open it owns the editing session, so the + model must not issue a duplicate write. +* ``search_audience_groups`` is read-only and visible to both, because the skill + needs it to resolve explicit audience names before an opener call. + +Nothing here logs announcement content, group identifiers, group names, group +mail, tokens, claims, or the opener request payload. +""" + +from __future__ import annotations + +import asyncio +import html +import json +import logging +import os +import time +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager +from datetime import datetime, timezone +from typing import Any, Literal, Optional +from urllib.parse import urlsplit + +import httpx +from mcp.server.fastmcp import FastMCP +from mcp.server.fastmcp.exceptions import ToolError +from mcp.types import CallToolResult, TextContent, ToolAnnotations +from portalocker.exceptions import LockException +from pydantic import ValidationError + +from client import ( + AgentConfigApiError, + BulletinValidationError, + IndeterminateWriteError, + OrgAnnouncementsClient, + build_manager_state, + is_deleted_item, + _validate_title_id, + _validate_bulletin_id, +) +from drafts import ( + AnnouncementEditorDraft, + AudienceGroup, + EditorMode, + SaveBulletinRequest, + SuggestedBulletinDraft, + OpenAnnouncementsRequest, + build_create_draft, + build_editor_draft_from_config, + without_blank_schedule, +) +from graph_directory_client import ( + GraphDirectoryClient, + GraphDirectoryError, + build_audience_metadata, + escape_search_value, +) +from telemetry import ( + SOURCE_BACKEND, + SOURCE_GRAPH, + SOURCE_MCP, + record_operation, +) + + +DEFAULT_WIDGET_ORIGIN = "https://workforceinsights.m365.cloud.microsoft" +WIDGET_MIME_TYPE = "text/html;profile=mcp-app" +ORG_ANNOUNCEMENTS_RESOURCE_URI = ( + "ui://widget/org-announcements/OrgAnnouncements.html" +) + +_LOGGER = logging.getLogger("ess-org-announcements") + +_READ_ONLY_ANNOTATIONS = ToolAnnotations( + readOnlyHint=True, + destructiveHint=False, + idempotentHint=True, + openWorldHint=False, +) +_MUTATION_ANNOTATIONS = ToolAnnotations( + readOnlyHint=False, + destructiveHint=False, + idempotentHint=False, + openWorldHint=False, +) +_DESTRUCTIVE_ANNOTATIONS = ToolAnnotations( + readOnlyHint=False, + destructiveHint=True, + idempotentHint=True, + openWorldHint=False, +) + +# Widget transition -> stored status. Publish now is intentionally absent: the +# scheduled row already holds canonical content and audience in the widget, so +# Vorpal calls save_bulletin with that complete state and only moves startDate +# to the current UTC instant. Routing it here would need a server-side +# GET-then-POST and would race a concurrent edit. +TRANSITION_STATUS = { + "archive": "retired", + "unarchive": "draft", + "moveToDraft": "draft", + "delete": "deleted", +} +TransitionName = Literal["archive", "unarchive", "moveToDraft", "delete"] + +# Backend codes that mean the tenant's OrgAnnouncementsSettings gate is off or +# the authoring surface is not deployed. These are reported as a recoverable +# unavailable state; the server never falls back to mock persistence. +# +# The list is intentionally exact-match and conservative. A substring or prefix +# rule would swallow a genuine field-level validation code that merely mentions +# a feature, turning a fixable "this value is wrong" into a dead-end "your +# tenant is not enabled" that the maker cannot act on. +_FEATURE_DISABLED_CODES = frozenset( + { + "FeatureDisabled", + "FeatureNotEnabled", + "OrgAnnouncementsDisabled", + "NotSupported", + } +) + +# The neutral client core synthesizes this code when a failing response carried +# no backend ``Code`` at all. It is a transport placeholder, NOT a backend +# validation code, so it must never be surfaced as one: the widget would look up +# localized copy for a code the backend never emitted, and a 500 would be +# reported to the maker as if they had typed something wrong. +_FALLBACK_BACKEND_CODE = "HttpError" + +# Identity and audit fields the backend owns. A duplicate strips them from the +# copied content so the copy is created as a fresh Draft rather than silently +# updating its source or inheriting its history. The wrapper-level audit fields +# (createdBy/createdOn/modifiedDate/status) are never copied at all, because the +# duplicate payload is rebuilt from content and audience only. +_COPY_STRIPPED_FIELDS = frozenset( + {"id", "createdBy", "createdOn", "modifiedDate", "status", "version", "etag"} +) + + +def _resolve_widget_origin(value: Optional[str] = None) -> str: + origin = ( + value or os.environ.get("VORPAL_WIDGET_ORIGIN") or DEFAULT_WIDGET_ORIGIN + ).rstrip("/") + parsed = urlsplit(origin) + if ( + parsed.scheme != "https" + or not parsed.hostname + or parsed.username is not None + or parsed.password is not None + or parsed.path + or parsed.query + or parsed.fragment + ): + raise ValueError( + "VORPAL_WIDGET_ORIGIN must be an HTTPS origin without credentials, " + "a path, a query, or a fragment." + ) + return origin + + +WIDGET_ORIGIN = _resolve_widget_origin() + + +def _widget_tool_meta() -> dict[str, Any]: + return { + "ui": { + "resourceUri": ORG_ANNOUNCEMENTS_RESOURCE_URI, + "visibility": ["model", "app"], + } + } + + +def _app_only_tool_meta() -> dict[str, Any]: + """Hide a mutation from the model so only the open widget can call it.""" + return {"ui": {"visibility": ["app"]}} + + +def _shared_tool_meta() -> dict[str, Any]: + return {"ui": {"visibility": ["model", "app"]}} + + +def _widget_resource_meta() -> dict[str, Any]: + return { + "ui": { + "domain": WIDGET_ORIGIN, + "csp": {"resourceDomains": [WIDGET_ORIGIN], "connectDomains": []}, + } + } + + +def _widget_shell() -> str: + script_url = html.escape( + f"{WIDGET_ORIGIN}/mcp-widget/org-announcements/widget.js", quote=True + ) + return ( + "\n" + '\n' + " \n" + ' \n' + ' \n' + " \n" + " \n" + '
\n' + f' \n' + " \n" + "\n" + ) + + +mcp = FastMCP( + "ess-org-announcements", + instructions=( + "Author ESS announcements for one deployed agent in the authenticated " + "tenant. A required titleId selects the agent, not an author permission " + "or an audience group. The current100/latest50 limits apply per tenant " + "and agent. Open the management view or the editor " + "through open_org_announcements; the widget owns every save, lifecycle " + "action, and audience search after it opens." + ), +) + +_client: Optional[OrgAnnouncementsClient] = None +_graph_client: Optional[GraphDirectoryClient] = None +_graph_client_users: dict[GraphDirectoryClient, int] = {} + +# Guards construction and invalidation of the process-global authoring client so +# concurrent tool calls share one sign-in instead of racing two browser prompts. +_client_lock = asyncio.Lock() + + +async def get_client() -> OrgAnnouncementsClient: + """Return the authoring client, constructing it lazily off the event loop. + + ``OrgAnnouncementsClient.__init__`` resolves a delegated token through the + shared core, which is synchronous and — on a cold cache — blocks for as long + as a human takes to finish an interactive browser sign-in. Constructing it + inline would run all of that *inside* the asyncio event loop, freezing every + other in-flight request and the MCP stdio transport itself for the duration. + + So construction happens in a worker thread, behind a lock, exactly like the + Graph client's token acquisition. + """ + global _client + if _client is not None: + return _client + async with _client_lock: + if _client is None: + try: + _client = await asyncio.to_thread(OrgAnnouncementsClient) + except (LockException, OSError) as error: + # Cache details stay on the exception cause, not in tool payloads. + raise _FailureResult( + "AuthenticationRequired", + "Organization announcement sign-in could not be completed. Try again.", + source=SOURCE_MCP, + ) from error + return _client + + +async def reset_client() -> None: + """Drop the authoring client so the next call reauthenticates. + + The shared core resolves its token once, in ``__init__``, and holds it for + the object's lifetime; there is no lazy re-acquisition inside it. Because + this server keeps one process-global client, an expired token would + otherwise fail every subsequent tool call until the MCP host was restarted. + Dropping the object is therefore the reauthentication mechanism: the next + ``get_client()`` builds a fresh one and re-runs silent-first MSAL, which + normally refreshes from the shared cache with no prompt. + """ + global _client + async with _client_lock: + client, _client = _client, None + # Closed outside the lock so teardown never blocks a concurrent rebuild. + if client is not None: + await client.aclose() + + +@asynccontextmanager +async def get_graph_client( + tenant_id: str, object_id: Optional[str] +) -> AsyncIterator[GraphDirectoryClient]: + """Capture one tenant-and-account-bound directory client for an operation. + + Keep only the current tenant's idle client. Replaced clients are closed + after their captured users finish, not while another request is using them. + The account comes from the captured authoring client, never from tool input. + """ + global _graph_client + previous = _graph_client + client = previous + if ( + client is None + or client.tenant_id != tenant_id + or client.object_id != object_id + ): + client = GraphDirectoryClient(tenant_id=tenant_id, object_id=object_id) + _graph_client = client + _graph_client_users[client] = _graph_client_users.get(client, 0) + 1 + try: + if ( + previous is not None + and previous is not client + and not _graph_client_users.get(previous) + ): + await previous.aclose() + yield client + finally: + remaining = _graph_client_users[client] - 1 + if remaining: + _graph_client_users[client] = remaining + else: + del _graph_client_users[client] + if client is not _graph_client: + await client.aclose() + + +def _now() -> datetime: + return datetime.now(timezone.utc) + + +def _elapsed_ms(started: float) -> int: + return int((time.monotonic() - started) * 1000) + + +def _text_result(payload: dict[str, Any], message: str) -> CallToolResult: + return CallToolResult( + content=[TextContent(type="text", text=message)], + structuredContent=payload, + ) + + +class _FailureResult(Exception): + """Carries a stable discriminated failure back to the tool boundary. + + Every MCP-only failure is one of the documented codes so the widget can + branch on ``code`` and ``retryable`` instead of parsing message text. + ``errors`` carries a *structured backend validation* payload when there is + one, so field-level codes survive intact instead of being flattened into a + single message. + """ + + def __init__( + self, + code: str, + message: str, + *, + retryable: bool = False, + field: Optional[str] = None, + source: str = SOURCE_MCP, + errors: Optional[list[dict[str, Any]]] = None, + ): + super().__init__(message) + self.code = code + self.message = message + self.retryable = retryable + self.field = field + self.source = source + self.errors = errors + + def as_error(self) -> dict[str, Any]: + return { + "code": self.code, + "field": self.field, + "message": self.message, + "retryable": self.retryable, + } + + def as_error_list(self) -> list[dict[str, Any]]: + """Every error to report, preserving a structured backend payload.""" + if self.errors: + return [ + { + "code": entry["code"], + "field": entry["field"], + "message": entry["message"], + "retryable": False, + } + for entry in self.errors + ] + return [self.as_error()] + + +def _validation_failure(error: BulletinValidationError) -> _FailureResult: + """Adapt an HTTP-200 ``EssBulletinSaveResult`` rejection. + + The wrapper's ``errors`` are carried through verbatim. The summary ``code`` + is the *first* backend code so a caller that only reads ``code`` still gets + a real backend code rather than a generic one, and ``field`` is that entry's + field so single-error cases keep their input binding. + """ + first = error.errors[0] + return _FailureResult( + first["code"], + first["message"], + field=first["field"], + retryable=False, + source=SOURCE_BACKEND, + errors=error.errors, + ) + + +def _classify_api_error(error: AgentConfigApiError) -> _FailureResult: + """Map a backend failure onto the documented discriminated results. + + Structured backend validation errors keep their own code and message: the + widget renders localized copy for ``AudienceRequired``, + ``AudienceGroupInvalid``, ``BulletinLimitExceeded``, and the field-level + title/description/action/date/priority/lifecycle codes, so replacing them + with a generic code would lose that mapping. + + The backend's own ``Code`` is inspected before the HTTP status so a + feature-gated tenant is reported as ``FeatureUnavailable`` rather than being + flattened into an authorization or not-found result — but only when the code + is a *real* backend code. ``HttpError`` is the core's placeholder for "the + response carried no code", so it is discarded before any code-based + decision. + """ + if isinstance(error, IndeterminateWriteError): + return _FailureResult("IndeterminateWrite", str(error), retryable=False) + if isinstance(error, BulletinValidationError): + return _validation_failure(error) + + raw_code, _, detail = str(error).partition(": ") + backend_code = "" if raw_code == _FALLBACK_BACKEND_CODE else raw_code + status = error.http_status + + if backend_code in _FEATURE_DISABLED_CODES and backend_code: + return _FailureResult( + "FeatureUnavailable", + "Organization announcements are not enabled for this tenant.", + source=SOURCE_BACKEND, + ) + if status == 401: + return _FailureResult( + "AuthenticationRequired", + "Sign in again to author organization announcements.", + source=SOURCE_BACKEND, + ) + if status == 403: + return _FailureResult( + "AuthorizationDenied", + "This account is not authorized to author organization " + "announcements in this tenant.", + source=SOURCE_BACKEND, + ) + if status == 404: + return _FailureResult( + "NotFound", + "The announcement was not found.", + source=SOURCE_BACKEND, + ) + if status in (400, 409, 422): + # A real backend validation failure. Without a backend code there is + # nothing to map, so report a generic invalid request rather than + # inventing one; the detail is the service's own message. + return _FailureResult( + backend_code or "InvalidRequest", + detail or str(error), + source=SOURCE_BACKEND, + ) + if status in (429, 502, 503, 504): + return _FailureResult( + "NetworkError", + "The Org Announcements service is temporarily unavailable.", + retryable=True, + source=SOURCE_BACKEND, + ) + if status in (405, 501): + return _FailureResult( + "FeatureUnavailable", + "Organization announcements are not enabled for this tenant.", + source=SOURCE_BACKEND, + ) + # Everything left is a server-side failure (500) or a response this client + # could not interpret. It is emphatically NOT a validation error the maker + # can fix, and the raw text can echo internal detail, so it becomes one + # stable, privacy-safe code. Non-retryable: the request reached the service + # and was processed, so an automatic replay would risk a duplicate write + # without any expectation of a different answer. + return _FailureResult( + "ServiceError", + "The Org Announcements service could not complete the request.", + retryable=False, + source=SOURCE_BACKEND, + ) + + +def _classify_graph_error(error: GraphDirectoryError) -> _FailureResult: + return _FailureResult( + error.code, + str(error), + retryable=error.retryable, + source=SOURCE_GRAPH, + ) + + +class _CommittedRefreshError(Exception): + """The write committed; only the follow-up refresh failed. + + Raised *after* the authoring API has definitely acknowledged a save, so the + announcement exists regardless of what happens next. It is deliberately a + distinct type from :class:`_FailureResult` because the two demand opposite + handling: a normal failure means "nothing was written, you may retry", and + this means "it was written, do not retry". + """ + + def __init__(self, cause: _FailureResult): + super().__init__(cause.message) + self.cause = cause + + +def _committed_refresh_failure(cause: _FailureResult) -> _FailureResult: + """Build the explicit partial-success contract for a committed write. + + Retrying here is not merely wasteful, it is unsafe: the create path is + unkeyed, so a replay would create a *second* announcement. The result is + therefore a stable, non-retryable ``CommittedRefreshFailed`` that states + plainly that the write succeeded and only the refresh did not, and the + original refresh failure is preserved as a second entry so the widget can + still tell a Graph outage from a service outage. + """ + summary_message = ( + "The change was saved, but the updated list could not be loaded. " + "Refresh to see the current state — do not repeat the action." + ) + return _FailureResult( + "CommittedRefreshFailed", + summary_message, + retryable=False, + source=cause.source, + errors=[ + { + "code": "CommittedRefreshFailed", + "field": None, + "message": summary_message, + }, + { + "code": cause.code, + "field": cause.field, + "message": cause.message, + }, + ], + ) + + +async def _resolve_audience_metadata( + graph_client: GraphDirectoryClient, + audience_ids: list[str], +) -> list[AudienceGroup]: + """Resolve one canonical audience list into ordered display metadata.""" + if not audience_ids: + return [] + try: + resolved = await graph_client.resolve_groups(audience_ids) + except GraphDirectoryError as error: + raise _FailureResult( + "AudienceMetadataUnavailable", + f"Audience group names could not be loaded. {error}", + retryable=error.retryable, + source=SOURCE_GRAPH, + ) from error + return [ + AudienceGroup.model_validate(item) + for item in build_audience_metadata(audience_ids, resolved) + ] + + +async def _resolve_manager_metadata( + graph_client: GraphDirectoryClient, + items: list[dict[str, Any]], +) -> dict[str, list[dict[str, Any]]]: + """Resolve audience metadata for every listed bulletin in one Graph batch. + + Soft-deleted rows are skipped: the manager drops them, so resolving their + audience would issue Graph lookups for groups nobody will see. + + Deduplication applies only to the lookup batch. Each bulletin's canonical + audience list is re-expanded in its own order, so per-item multiplicity and + ordering stay exact. + """ + visible = [config for config in items if not is_deleted_item(config)] + + all_ids: list[str] = [] + for config in visible: + audience = config.get("audience") + if isinstance(audience, list): + all_ids.extend( + group_id for group_id in audience if isinstance(group_id, str) + ) + + if not all_ids: + return {} + + try: + resolved = await graph_client.resolve_groups(all_ids) + except GraphDirectoryError as error: + raise _FailureResult( + "AudienceMetadataUnavailable", + f"Audience group names could not be loaded. {error}", + retryable=error.retryable, + source=SOURCE_GRAPH, + ) from error + + metadata: dict[str, list[dict[str, Any]]] = {} + for config in visible: + bulletin = config.get("bulletin") + bulletin_id = bulletin.get("id") if isinstance(bulletin, dict) else None + audience = config.get("audience") + metadata[bulletin_id or ""] = build_audience_metadata( + [group_id for group_id in audience if isinstance(group_id, str)] + if isinstance(audience, list) + else [], + resolved, + ) + return metadata + + +async def _manager_state( + client: OrgAnnouncementsClient, scope: dict[str, str], + graph_client: GraphDirectoryClient, +) -> dict[str, Any]: + items = await client.list_bulletins(scope["titleId"]) + return build_manager_state( + items, + await _resolve_manager_metadata(graph_client, items), + _now(), + tenant_id=scope["tenantId"], + title_id=scope["titleId"], + ) + + +def _editor_payload( + mode: EditorMode, + config: Optional[dict[str, Any]], + draft: AnnouncementEditorDraft, + scope: dict[str, str], +) -> dict[str, Any]: + return { + **scope, + "view": "editor", + "mode": mode, + "config": config, + "draft": draft.model_dump( + mode="json", exclude={"id"} if draft.id is None else set() + ), + } + + +def _open_error_payload( + request: dict[str, Any], failure: _FailureResult, scope: dict[str, str] +) -> CallToolResult: + """Return a recoverable open error instead of a half-rendered editor. + + The request is response-only retry state, including the original suggestion. + It is never logged or sent to telemetry. + """ + payload = { + **scope, + "view": "error", + "request": request, + "code": failure.code, + "message": failure.message, + "retryable": failure.retryable, + } + return CallToolResult( + content=[TextContent(type="text", text=failure.message)], + structuredContent=payload, + isError=True, + ) + + +def _open_success( + payload: dict[str, Any], message: str, started: float +) -> CallToolResult: + record_operation( + "open_org_announcements", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + return _text_result(payload, message) + + +def _open_failure( + request: dict[str, Any], + failure: _FailureResult, + started: float, + scope: dict[str, str], +) -> CallToolResult: + _LOGGER.warning("open_org_announcements failed: %s", failure.code) + record_operation( + "open_org_announcements", + outcome="failure", + latency_ms=_elapsed_ms(started), + error_code=failure.code, + error_source=failure.source, + ) + return _open_error_payload(request, failure, scope) + + +@mcp.resource( + ORG_ANNOUNCEMENTS_RESOURCE_URI, + name="Org announcements", + title="Org announcements", + description="Announcement manager and editor for the selected ESS agent.", + mime_type=WIDGET_MIME_TYPE, + meta=_widget_resource_meta(), +) +def org_announcements_widget() -> str: + return _widget_shell() + + +@mcp.tool( + annotations=_READ_ONLY_ANNOTATIONS, +) +async def list_agent_configs() -> str: + """List configured deployed ESS agents; never initialize configuration.""" + client = await get_client() + return json.dumps(await client.list_agent_configs(), indent=2) + + +@mcp.tool( + annotations=_READ_ONLY_ANNOTATIONS, +) +async def search_agents(searchString: str) -> str: + """Find deployed ESS agents by name to resolve their titleId.""" + client = await get_client() + return json.dumps(await client.search_agents(searchString), indent=2) + + +@mcp.tool( + meta=_widget_tool_meta(), + annotations=_READ_ONLY_ANNOTATIONS, +) +async def open_org_announcements( + titleId: str, + view: Literal["manager", "editor"], + mode: Optional[Literal["create", "edit"]] = None, + bulletinId: Optional[str] = None, + suggestedDraft: Optional[SuggestedBulletinDraft] = None, +) -> CallToolResult: + """Open announcements for the selected agent in the manager or editor. + + This tool only reads: it never creates, updates, or publishes. The model + calls it at most once per maker turn; the widget uses it for navigation + within the same tenant-and-agent scope. Manager takes no editor arguments. + Editor requires mode=create without bulletinId, or mode=edit with bulletinId. + suggestedDraft is supported only for create. + """ + # The flat tool signature stays compatible with MCP callers. Validate the + # combination before entering the recoverable widget-error path: an invalid + # request is not safe retry state and cannot be rendered as an error view. + try: + _validate_title_id(titleId) + if bulletinId is not None: + _validate_bulletin_id(bulletinId) + validated = OpenAnnouncementsRequest( + titleId=titleId, view=view, mode=mode, + bulletinId=bulletinId, suggestedDraft=suggestedDraft, + ) + except ValidationError as error: + messages = "; ".join( + entry["msg"] for entry in error.errors(include_input=False) + ) + raise ToolError(f"Invalid announcement opener: {messages}") from None + except ValueError as error: + raise ToolError(f"Invalid announcement opener: {error}") from None + + started = time.monotonic() + scope = {"titleId": titleId} + request = validated.model_dump(exclude_none=True, exclude={"suggestedDraft"}) + if suggestedDraft is not None: + request["suggestedDraft"] = suggestedDraft.retry_payload() + + try: + # Capture once even for an empty create. Subsequent reads and post-save + # refreshes must never adopt a different global client's tenant. + client = await get_client() + scope = {"tenantId": client.tenant_id, "titleId": titleId} + + if view == "manager": + async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: + state = await _manager_state(client, scope, graph_client) + return _open_success( + {"view": "manager", **state}, + "Opened announcements for the selected ESS agent.", + started, + ) + + if mode == "create": + async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: + audience_metadata = await _resolve_audience_metadata( + graph_client, + list(suggestedDraft.audience) + if suggestedDraft is not None and suggestedDraft.audience + else [], + ) + draft = build_create_draft(suggestedDraft, audience_metadata) + return _open_success( + _editor_payload("create", None, draft, scope), + "Opened a new organization announcement for review. Nothing is " + "saved until you publish or save a draft in the editor.", + started, + ) + + config = await client.get_bulletin(titleId, bulletinId) + audience = config.get("audience") + async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: + audience_metadata = await _resolve_audience_metadata( + graph_client, + [group_id for group_id in audience if isinstance(group_id, str)] + if isinstance(audience, list) + else [], + ) + draft = build_editor_draft_from_config(config, audience_metadata) + message = "Opened the announcement for editing." + return _open_success( + _editor_payload("edit", config, draft, scope), message, started + ) + + except _FailureResult as failure: + return _open_failure(request, failure, started, scope) + except AgentConfigApiError as error: + return _open_failure(request, await _failure_from(error), started, scope) + except httpx.RequestError: + return _open_failure( + request, + _FailureResult( + "NetworkError", + "The Org Announcements service could not be reached.", + retryable=True, + source=SOURCE_BACKEND, + ), + started, + scope, + ) + except (ValidationError, ValueError) as error: + return _open_failure( + request, _FailureResult("InvalidRequest", str(error)), started, scope + ) + + +def _fail( + operation: str, failure: _FailureResult, started: float, scope: dict[str, str] +) -> CallToolResult: + """Emit the content-free failure event and build the tool result.""" + record_operation( + operation, + outcome="failure", + latency_ms=_elapsed_ms(started), + error_code=failure.code, + error_source=failure.source, + ) + return _mutation_failure(failure, scope) + + +def _mutation_failure( + failure: _FailureResult, scope: dict[str, str] +) -> CallToolResult: + return CallToolResult( + content=[TextContent(type="text", text=failure.message)], + structuredContent={ + **scope, "status": "failure", "errors": failure.as_error_list() + }, + isError=True, + ) + + +async def _failure_from(error: Exception) -> _FailureResult: + """Classify a failure and reauthenticate the authoring client on a 401. + + A 401 from the authoring API means the cached delegated token is expired or + revoked. The shared core holds its token for the client object's lifetime, + so the stale credential is discarded by discarding the client; the next tool + call then rebuilds it and re-runs silent-first MSAL. + + Only an *authoring-side* 401 resets the authoring client. The Graph client + reports the same ``AuthenticationRequired`` code for its own expiry and + handles that itself, so the source bucket is what distinguishes them — + resetting on a Graph 401 would throw away a perfectly good WeveNova token. + """ + failure = _to_failure(error) + if failure.code == "AuthenticationRequired" and failure.source == SOURCE_BACKEND: + await reset_client() + return failure + + +def _to_failure(error: Exception) -> _FailureResult: + if isinstance(error, _FailureResult): + return error + if isinstance(error, AgentConfigApiError): + return _classify_api_error(error) + if isinstance(error, GraphDirectoryError): + return _classify_graph_error(error) + if isinstance(error, httpx.RequestError): + return _FailureResult( + "NetworkError", + "The Org Announcements service could not be reached.", + retryable=True, + source=SOURCE_BACKEND, + ) + return _FailureResult("InvalidRequest", str(error)) + + +# Everything a mutation can raise. Listed once so the write step and the +# post-commit refresh step cannot drift apart and let an exception escape one +# but not the other. +_MUTATION_ERRORS = ( + _FailureResult, + AgentConfigApiError, + GraphDirectoryError, + httpx.RequestError, + ValueError, +) + + +async def _saved_item_result( + client: OrgAnnouncementsClient, + scope: dict[str, str], + config: dict[str, Any], + message: str, +) -> CallToolResult: + """Return the canonical saved item plus refreshed manager state. + + Called only after the write has been acknowledged, so any error raised here + is a *refresh* failure over a committed record. It is wrapped in + :class:`_CommittedRefreshError` so the caller cannot mistake it for a failed + write and offer a retry that would duplicate the announcement. + """ + try: + audience = config.get("audience") + async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: + audience_metadata = await _resolve_audience_metadata( + graph_client, + [group_id for group_id in audience if isinstance(group_id, str)] + if isinstance(audience, list) + else [], + ) + manager = await _manager_state(client, scope, graph_client) + except _MUTATION_ERRORS as error: + raise _CommittedRefreshError(await _failure_from(error)) from error + + return _text_result( + { + **scope, + "status": "success", + "item": { + "config": config, + "audienceMetadata": [ + group.model_dump(mode="json") for group in audience_metadata + ], + }, + "manager": manager, + }, + message, + ) + + +@mcp.tool( + meta=_app_only_tool_meta(), + annotations=_MUTATION_ANNOTATIONS, +) +async def save_bulletin( + titleId: str, + bulletin: dict[str, Any], + audience: list[str], + status: Literal["draft", "published"], + id: Optional[str] = None, # noqa: A002 — the widget's wire field name +) -> CallToolResult: + """Create or update an announcement with its complete authored state.""" + started = time.monotonic() + scope = {"titleId": titleId} + try: + _validate_title_id(titleId) + request = SaveBulletinRequest.model_validate( + { + "id": id, + "bulletin": bulletin, + "audience": audience, + "status": status, + } + ) + except ValueError as error: + return _fail( + "save_bulletin", + _FailureResult("InvalidRequest", str(error)), + started, + scope, + ) + + payload = request.model_dump(mode="json", exclude_none=True) + try: + client = await get_client() + scope = {"tenantId": client.tenant_id, "titleId": titleId} + saved = await client.save_bulletin(titleId, payload) + except _MUTATION_ERRORS as error: + failure = await _failure_from(error) + _LOGGER.warning( + "save_bulletin failed: %s (create=%s)", failure.code, id is None + ) + return _fail("save_bulletin", failure, started, scope) + + try: + result = await _saved_item_result( + client, + scope, + saved, + "Published the organization announcement." + if status == "published" + else "Saved the organization announcement draft.", + ) + except _CommittedRefreshError as error: + # The write is committed. Report the explicit partial success so the + # widget tells the maker to refresh instead of offering a retry that + # would create a second announcement. + _LOGGER.warning( + "save_bulletin refresh failed after commit: %s", error.cause.code + ) + return _fail( + "save_bulletin", _committed_refresh_failure(error.cause), started, scope + ) + + record_operation( + "save_bulletin", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + return result + + +@mcp.tool( + meta=_app_only_tool_meta(), + annotations=_DESTRUCTIVE_ANNOTATIONS, +) +async def transition_bulletin( + titleId: str, + id: str, # noqa: A002 — the widget's wire field name + transition: TransitionName, +) -> CallToolResult: + """Archive, unarchive, move to draft, or delete an announcement. + + The request carries only the identifier and the new status. The service + loads the canonical record, preserves its authored content and audience, and + validates the lifecycle change, so no client-side merge is performed. + + ``TransitionName`` lists exactly the four supported operations. Publish now + is deliberately absent and is rejected at argument validation: it must go + through ``save_bulletin`` with the row's complete canonical state so the new + start instant cannot race a concurrent edit. + + KNOWN LIMITATION (Vorpal compatibility). A legacy client sending + ``transition: "publishNow"`` is rejected by FastMCP's *schema* validation, + before this function runs, so it surfaces as a protocol ``ToolError`` rather + than this server's structured ``InvalidRequest`` result. Converting it would + require widening ``transition`` to a free string, which would advertise + ``publishNow`` as acceptable in the production schema and re-open the race + that removing it closed — so the rollout prerequisite stands: ship a Vorpal + build that routes publish-now through ``save_bulletin``. The current + boundary is pinned by tests so it stays a known contract. + """ + started = time.monotonic() + scope = {"titleId": titleId} + try: + _validate_title_id(titleId) + client = await get_client() + scope = {"tenantId": client.tenant_id, "titleId": titleId} + changed = await client.transition_bulletin( + titleId, id, TRANSITION_STATUS[transition] + ) + except _MUTATION_ERRORS as error: + failure = await _failure_from(error) + _LOGGER.warning( + "transition_bulletin failed: %s (%s)", failure.code, transition + ) + return _fail("transition_bulletin", failure, started, scope) + + # The transition is committed from here on. A refresh failure must never be + # reported as a retryable normal failure, because the lifecycle change has + # already been applied and re-issuing it could fail validation or move the + # record again. + try: + async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: + manager = await _manager_state(client, scope, graph_client) + except _MUTATION_ERRORS as error: + cause = await _failure_from(error) + _LOGGER.warning( + "transition_bulletin refresh failed after commit: %s (%s)", + cause.code, + transition, + ) + return _fail( + "transition_bulletin", _committed_refresh_failure(cause), started, scope + ) + + record_operation( + "transition_bulletin", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + # The canonical changed row is included alongside the manager state. It is + # additive: the widget's existing manager-shaped contract is untouched, so a + # host that strips unknown fields simply ignores ``item`` and still gets a + # correct refresh. + return _text_result( + { + **scope, "status": "success", + "item": {"config": changed}, "manager": manager, + }, + "Updated the organization announcement.", + ) + + +@mcp.tool( + meta=_app_only_tool_meta(), + annotations=_MUTATION_ANNOTATIONS, +) +async def duplicate_bulletin( + titleId: str, + id: str, # noqa: A002 — the widget's wire field name +) -> CallToolResult: + """Copy an existing announcement into a new Draft. + + Loads the canonical source, strips its identity and audit fields, and + creates a new Draft. A missing source is a not-found failure, never a + create: duplicating something that no longer exists must not invent a + record. + """ + started = time.monotonic() + scope = {"titleId": titleId} + try: + _validate_title_id(titleId) + client = await get_client() + scope = {"tenantId": client.tenant_id, "titleId": titleId} + source = await client.get_bulletin(titleId, id) + except _MUTATION_ERRORS as error: + failure = await _failure_from(error) + _LOGGER.warning("duplicate_bulletin source load failed: %s", failure.code) + return _fail("duplicate_bulletin", failure, started, scope) + + # Stored content is forwarded verbatim, so it never passes through + # BulletinInput. The blank-schedule sentinel is stripped explicitly here for + # the same reason it is coerced there: "" is not a DateTimeOffset, and + # sending it would fail model binding on a copy the maker never edited. + bulletin = without_blank_schedule( + { + key: value + for key, value in source["bulletin"].items() + if key not in _COPY_STRIPPED_FIELDS + } + ) + audience = source.get("audience") + payload = { + "bulletin": bulletin, + "audience": [ + group_id + for group_id in (audience if isinstance(audience, list) else []) + if isinstance(group_id, str) + ], + "status": "draft", + } + + try: + created = await client.save_bulletin(titleId, payload) + except _MUTATION_ERRORS as error: + failure = await _failure_from(error) + _LOGGER.warning("duplicate_bulletin failed: %s", failure.code) + return _fail("duplicate_bulletin", failure, started, scope) + + try: + result = await _saved_item_result( + client, scope, created, + "Created a draft copy of the organization announcement.", + ) + except _CommittedRefreshError as error: + # The copy exists. This is an unkeyed create, so a retry would produce a + # second copy; report the committed partial success instead. + _LOGGER.warning( + "duplicate_bulletin refresh failed after commit: %s", error.cause.code + ) + return _fail( + "duplicate_bulletin", _committed_refresh_failure(error.cause), started, scope + ) + + record_operation( + "duplicate_bulletin", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + return result + + +@mcp.tool( + meta=_shared_tool_meta(), + annotations=_READ_ONLY_ANNOTATIONS, +) +async def search_audience_groups(query: str) -> CallToolResult: + """Search eligible audience groups by display name. + + Returns at most 20 security groups, mail-enabled security groups, or classic + distribution groups, deduplicated by ID and in a deterministic order. + Microsoft 365 groups and dynamic-membership groups are not offered. + + ``exhausted`` and ``pagesExamined`` are reported so the caller can tell "no + more matches exist" from "the page budget ran out". Without them a capped + search looks identical to an exhausted one and the maker would be told a + group does not exist when it simply was not reached. + """ + started = time.monotonic() + try: + escape_search_value(query) + authoring_client = await get_client() + async with get_graph_client( + authoring_client.tenant_id, authoring_client.object_id + ) as graph_client: + result = await graph_client.search_groups(query) + except (_FailureResult, GraphDirectoryError, AgentConfigApiError, httpx.RequestError) as error: + failure = await _failure_from(error) + _LOGGER.warning("search_audience_groups failed: %s", failure.code) + record_operation( + "search_audience_groups", + outcome="failure", + latency_ms=_elapsed_ms(started), + error_code=failure.code, + error_source=failure.source, + ) + return CallToolResult( + content=[TextContent(type="text", text=failure.message)], + structuredContent={ + "status": "failure", + "code": failure.code, + "retryable": failure.retryable, + }, + isError=True, + ) + except ValueError as error: + _LOGGER.warning("search_audience_groups rejected an invalid query") + record_operation( + "search_audience_groups", + outcome="failure", + latency_ms=_elapsed_ms(started), + error_code="InvalidRequest", + error_source=SOURCE_MCP, + ) + return CallToolResult( + content=[TextContent(type="text", text=str(error))], + structuredContent={ + "status": "failure", + "code": "InvalidRequest", + "retryable": False, + }, + isError=True, + ) + + groups = result["groups"] + record_operation( + "search_audience_groups", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + return _text_result( + { + "status": "success", + "groups": groups, + "exhausted": result["exhausted"], + "pagesExamined": result["pagesExamined"], + }, + f"Found {len(groups)} eligible audience group(s).", + ) + + +if __name__ == "__main__": + mcp.run() diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py new file mode 100644 index 000000000..15bce2f31 --- /dev/null +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py @@ -0,0 +1,156 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Content-free telemetry for the Org Announcements MCP server. + +This is a thin, deliberately narrow adapter over the kit's existing +``scripts/adk_telemetry.py`` conventions. It emits one ``adk.api.call`` event +per tool invocation carrying exactly four things: + +* ``operation`` — the MCP tool name, from a fixed allowlist; +* ``outcome`` — ``success`` or ``failure``; +* ``latency_ms`` — wall-clock duration of the operation; +* ``error_code`` — one of this server's stable discriminated codes; +* ``error_category`` — a broad source bucket (``backend``/``graph``/``mcp``). + +Nothing else. In particular this module never emits, and has no parameter that +could carry, announcement content (title, description, action label, URL, or +prompt), audience group identifiers or names, the tenant's API endpoint, tokens, +claims, or the opener request payload. ``error_message`` is deliberately never +populated: backend messages can echo authored content back, and the stable code +is what a dashboard or a support engineer actually needs. + +Every path fails open. Telemetry must never turn a working save into a failed +tool call, so import errors, missing config, and emit errors are all swallowed. +""" + +from __future__ import annotations + +import logging +import os +import sys +from typing import Any, Optional + + +_LOGGER = logging.getLogger("ess-org-announcements.telemetry") + +# The MCP tool names this server exposes. An operation outside the allowlist is +# bucketed rather than emitted verbatim, so a future tool cannot silently mint a +# new dimension value (and cannot smuggle caller-controlled text into Aria). +_OPERATIONS = frozenset( + { + "open_org_announcements", + "save_bulletin", + "transition_bulletin", + "duplicate_bulletin", + "search_audience_groups", + } +) +OPERATION_UNKNOWN = "unknown" + +# Broad source buckets. Anything narrower would start describing the tenant's +# configuration. +SOURCE_BACKEND = "backend" +SOURCE_GRAPH = "graph" +SOURCE_MCP = "mcp" +_SOURCES = frozenset({SOURCE_BACKEND, SOURCE_GRAPH, SOURCE_MCP}) + +# Stable codes are short identifiers, never free text. This bound is a +# belt-and-braces guard so a malformed code can never carry a payload. +_MAX_CODE_LENGTH = 64 + +# Emitting is opt-out through the same switch the rest of the ADK honours; the +# module-level import is resolved lazily so a server started outside the kit +# layout still runs. +_ADK_TELEMETRY: Optional[Any] = None +_ADK_TELEMETRY_RESOLVED = False + +_SCRIPTS_DIR = os.path.abspath( + os.path.join( + os.path.dirname(os.path.abspath(__file__)), "..", "..", "..", "scripts" + ) +) + + +def normalize_operation(operation: str) -> str: + """Clamp an operation name to the allowlist.""" + if not isinstance(operation, str): + return OPERATION_UNKNOWN + return operation if operation in _OPERATIONS else OPERATION_UNKNOWN + + +def normalize_source(source: str) -> str: + """Clamp an error source to the broad bucket allowlist.""" + if not isinstance(source, str): + return SOURCE_MCP + return source if source in _SOURCES else SOURCE_MCP + + +def normalize_error_code(error_code: str) -> str: + """Keep only a short, identifier-shaped stable code. + + A backend code arrives as an identifier such as ``AudienceGroupInvalid``. + Anything containing whitespace or punctuation is a message, not a code, so + it is replaced rather than truncated — a truncated message is still content. + """ + if not isinstance(error_code, str): + return "" + candidate = error_code.strip() + if not candidate: + return "" + if len(candidate) > _MAX_CODE_LENGTH or not candidate.replace("_", "").isalnum(): + return "UnknownError" + return candidate + + +def _adk_telemetry() -> Optional[Any]: + """Resolve ``scripts/adk_telemetry`` once, tolerating its absence.""" + global _ADK_TELEMETRY, _ADK_TELEMETRY_RESOLVED + if _ADK_TELEMETRY_RESOLVED: + return _ADK_TELEMETRY + _ADK_TELEMETRY_RESOLVED = True + try: + if _SCRIPTS_DIR not in sys.path: + sys.path.append(_SCRIPTS_DIR) + import adk_telemetry # noqa: PLC0415 — resolved lazily and optionally + + _ADK_TELEMETRY = adk_telemetry + except Exception: # noqa: BLE001 — telemetry must never break a tool call + _ADK_TELEMETRY = None + return _ADK_TELEMETRY + + +def record_operation( + operation: str, + *, + outcome: str, + latency_ms: int, + error_code: str = "", + error_source: str = SOURCE_MCP, +) -> None: + """Emit one content-free ``adk.api.call`` event for a tool invocation. + + ``api_endpoint`` carries the *tool name*, not a URL: the tenant's API host + is environment-specific and is never reported. Failure adds only the stable + code and the broad source bucket; ``error_message`` is left empty on purpose. + """ + telemetry = _adk_telemetry() + if telemetry is None: + return + + normalized_outcome = "success" if outcome == "success" else "failure" + fields: dict[str, Any] = { + "api_endpoint": normalize_operation(operation), + "outcome": normalized_outcome, + "latency_ms": max(0, int(latency_ms)), + } + if normalized_outcome == "failure": + fields["error_code"] = normalize_error_code(error_code) + fields["error_category"] = normalize_source(error_source) + # Never a message: backend text can echo the announcement back. + fields["error_message"] = "" + + try: + telemetry.emit_api_call(**fields) + except Exception: # noqa: BLE001 — fail open, never break the tool call + _LOGGER.debug("Org Announcements telemetry emit failed", exc_info=False) diff --git a/tests/mcp/agentconfig_core/test_agent_discovery.py b/tests/mcp/agentconfig_core/test_agent_discovery.py index 4836df87f..688979c7d 100644 --- a/tests/mcp/agentconfig_core/test_agent_discovery.py +++ b/tests/mcp/agentconfig_core/test_agent_discovery.py @@ -18,9 +18,11 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from _mcp_modules import ( # noqa: E402 load_landing_page_modules, + load_org_announcements_modules, ) LANDING = load_landing_page_modules() +ANNOUNCEMENTS = load_org_announcements_modules() from agent_discovery import AgentDiscoveryClient # noqa: E402 from base_client import AgentConfigApiError # noqa: E402 @@ -28,6 +30,7 @@ TENANT = "00000000-0000-0000-0000-000000001111" CLIENTS = [ LANDING["client"].AgentConfigClient, + ANNOUNCEMENTS["client"].OrgAnnouncementsClient, ] @@ -104,7 +107,7 @@ async def run(): asyncio.run(run()) -@pytest.mark.parametrize("modules", [LANDING]) +@pytest.mark.parametrize("modules", [LANDING, ANNOUNCEMENTS]) def test_feature_owned_tools_delegate_without_initializing_configuration(monkeypatch, modules): calls = [] @@ -117,10 +120,13 @@ async def search_agents(self, query): calls.append(("search", query)) return [{"titleId": "deployed-title"}] + async def get_async_client(): + return DiscoveryOnly() + server = modules["server"] monkeypatch.setattr( server, "get_client", - lambda: DiscoveryOnly(), + get_async_client if modules is ANNOUNCEMENTS else lambda: DiscoveryOnly(), ) async def run(): @@ -129,9 +135,12 @@ async def run(): tools = {tool.name: tool for tool in await server.mcp.list_tools()} for name, properties in [("list_agent_configs", set()), ("search_agents", {"searchString"})]: assert set(tools[name].inputSchema["properties"]) == properties - # Preserve the existing provider's metadata as well as its wire - # arguments; request-level tests establish read-only behavior. - assert tools[name].annotations is None + if modules is ANNOUNCEMENTS: + assert tools[name].annotations.readOnlyHint is True + else: + # Preserve the sibling's existing tool metadata as well as its + # wire arguments; request-level tests establish read-only behavior. + assert tools[name].annotations is None assert not (tools[name].meta or {}).get("ui", {}).get("resourceUri") asyncio.run(run()) diff --git a/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py b/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py new file mode 100644 index 000000000..382a9d002 --- /dev/null +++ b/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py @@ -0,0 +1,433 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Lifecycle guards for the process-global AgentConfiguration authoring client. + +Three properties are load-bearing for an MCP server and are asserted here +because none of them is visible from the tool contracts: + +* construction must not run on the asyncio event loop, because the shared core + resolves a delegated token synchronously and can block on a human; +* nothing may reach stdout, because stdout is the JSON-RPC transport; and +* an expired token must be recoverable without restarting the MCP host. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import io +import sys +import threading +from pathlib import Path +from typing import Any + +import pytest + + +REPO_ROOT = Path(__file__).resolve().parents[3] + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from _mcp_modules import load_org_announcements_modules # noqa: E402 + +_ORG_MODULES = load_org_announcements_modules() +org_server = _ORG_MODULES["server"] +org_client = _ORG_MODULES["client"] + + +@pytest.fixture(autouse=True) +def _reset_globals(): + """Never leak a fake client between tests or into another module.""" + org_server._client = None + yield + org_server._client = None + + +class _FakeAuthoringClient: + def __init__(self) -> None: + self.closed = False + + async def aclose(self) -> None: + self.closed = True + + async def list_bulletins(self) -> list[dict[str, Any]]: + return [] + + +# -------------------------------------------------------------------------- +# Lazy, off-loop construction +# -------------------------------------------------------------------------- + + +def test_importing_the_server_constructs_no_authoring_client() -> None: + """Import must be side-effect free; a token prompt at import is fatal.""" + assert org_server._client is None + + +def test_the_authoring_client_is_constructed_off_the_event_loop( + monkeypatch, +) -> None: + """The shared core resolves its token synchronously in ``__init__``. + + On a cold cache that blocks for as long as a human takes to finish a browser + sign-in. Running it inline would freeze the event loop — and with it every + other in-flight request and the MCP stdio transport. + """ + threads: list[int] = [] + + def _construct() -> _FakeAuthoringClient: + threads.append(threading.get_ident()) + return _FakeAuthoringClient() + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + + async def run() -> int: + loop_thread = threading.get_ident() + await org_server.get_client() + return loop_thread + + loop_thread = asyncio.run(run()) + + assert len(threads) == 1 + assert threads[0] != loop_thread, "the client was built on the event loop" + + +def test_the_authoring_client_is_built_once_and_reused(monkeypatch) -> None: + constructions: list[int] = [] + + def _construct() -> _FakeAuthoringClient: + constructions.append(1) + return _FakeAuthoringClient() + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + + async def run() -> None: + first = await org_server.get_client() + second = await org_server.get_client() + assert first is second + + asyncio.run(run()) + + assert constructions == [1] + + +def test_concurrent_first_calls_share_one_construction(monkeypatch) -> None: + """Two tool calls at once must not race two interactive sign-ins.""" + constructions: list[int] = [] + + def _construct() -> _FakeAuthoringClient: + constructions.append(1) + return _FakeAuthoringClient() + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + + async def run() -> list: + return await asyncio.gather( + org_server.get_client(), + org_server.get_client(), + org_server.get_client(), + ) + + clients = asyncio.run(run()) + + assert constructions == [1] + assert clients[0] is clients[1] is clients[2] + + +# -------------------------------------------------------------------------- +# Reauthentication after a 401 +# -------------------------------------------------------------------------- + + +def test_reset_drops_and_closes_the_authoring_client(monkeypatch) -> None: + monkeypatch.setattr( + org_server, "OrgAnnouncementsClient", _FakeAuthoringClient + ) + + async def run() -> _FakeAuthoringClient: + client = await org_server.get_client() + await org_server.reset_client() + assert org_server._client is None + return client + + client = asyncio.run(run()) + + assert client.closed, "the stale client was dropped without being closed" + + +def test_the_call_after_a_reset_builds_a_fresh_client(monkeypatch) -> None: + """Rebuilding is the reauthentication mechanism. + + The shared core resolves its token once, in ``__init__``, and never + refreshes it, so a new object is what re-runs silent-first MSAL. + """ + built: list[_FakeAuthoringClient] = [] + + def _construct() -> _FakeAuthoringClient: + client = _FakeAuthoringClient() + built.append(client) + return client + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + + async def run() -> None: + first = await org_server.get_client() + await org_server.reset_client() + second = await org_server.get_client() + assert first is not second + + asyncio.run(run()) + + assert len(built) == 2 + + +def test_an_authoring_401_resets_the_client(monkeypatch) -> None: + """A stale token must not fail every later tool call until restart.""" + monkeypatch.setattr( + org_server, "OrgAnnouncementsClient", _FakeAuthoringClient + ) + + async def run() -> None: + client = await org_server.get_client() + failure = await org_server._failure_from( + org_client.AgentConfigApiError("HttpError: HTTP 401", http_status=401) + ) + assert failure.code == "AuthenticationRequired" + assert org_server._client is None + assert client.closed + + asyncio.run(run()) + + +def test_a_graph_401_does_not_reset_the_authoring_client(monkeypatch) -> None: + """Both surfaces report ``AuthenticationRequired``; only one should reset. + + The Graph client handles its own expiry, so resetting here would throw away + a perfectly good WeveNova token and force a needless second sign-in. + """ + monkeypatch.setattr( + org_server, "OrgAnnouncementsClient", _FakeAuthoringClient + ) + graph_error = _ORG_MODULES["graph_directory_client"].GraphDirectoryError( + "graph token expired", code="AuthenticationRequired", retryable=False + ) + + async def run() -> None: + client = await org_server.get_client() + failure = await org_server._failure_from(graph_error) + assert failure.code == "AuthenticationRequired" + assert failure.source == org_server.SOURCE_GRAPH + assert org_server._client is client + assert not client.closed + + asyncio.run(run()) + + +@pytest.mark.parametrize("status", [403, 404, 429, 500, 503]) +def test_non_401_authoring_failures_keep_the_client(monkeypatch, status) -> None: + monkeypatch.setattr( + org_server, "OrgAnnouncementsClient", _FakeAuthoringClient + ) + + async def run() -> None: + client = await org_server.get_client() + await org_server._failure_from( + org_client.AgentConfigApiError( + f"HttpError: HTTP {status}", http_status=status + ) + ) + assert org_server._client is client + assert not client.closed + + asyncio.run(run()) + + +def test_resetting_when_no_client_exists_is_a_no_op() -> None: + async def run() -> None: + await org_server.reset_client() + assert org_server._client is None + + asyncio.run(run()) + + +# -------------------------------------------------------------------------- +# stdout purity +# -------------------------------------------------------------------------- + + +def _fake_msal(monkeypatch, tmp_path, *, interactive: bool): + """Patch MSAL so the shared core runs its sign-in path without a browser. + + The interactive path is exercised for real up to and including the sign-in + notice — that notice is what these tests are about — so the loopback + listener is replaced with a fake that completes immediately instead of + blocking on a callback that will never arrive. + """ + import base_client + + class _FakeApp: + def __init__(self, *args: Any, **kwargs: Any) -> None: + pass + + def get_accounts(self): + return [] if interactive else [{"username": "maker@contoso.com"}] + + def acquire_token_silent(self, *args: Any, **kwargs: Any): + return None if interactive else {"access_token": _fake_jwt()} + + def initiate_auth_code_flow(self, *args: Any, **kwargs: Any): + return {"auth_uri": "https://login.microsoftonline.com/fake"} + + def acquire_token_by_auth_code_flow(self, *args: Any, **kwargs: Any): + return {"access_token": _fake_jwt()} + + class _FakeCache: + has_state_changed = False + + def deserialize(self, data: str) -> None: + pass + + def serialize(self) -> str: + return "" + + class _FakeHTTPServer: + """Stands in for the loopback form_post listener.""" + + server_port = 54321 + + def __init__(self, address, handler) -> None: + self._handler = handler + + def handle_request(self) -> None: + self._handler.captured = {"code": "fake-auth-code"} + + def server_close(self) -> None: + pass + + import msal + + monkeypatch.setattr(msal, "PublicClientApplication", _FakeApp) + monkeypatch.setattr(base_client, "create_token_cache", lambda path: _FakeCache()) + monkeypatch.setattr(base_client, "_TOKEN_CACHE_PATH", str(tmp_path / "c.bin")) + monkeypatch.setattr(base_client, "_LOCAL_STATE_DIR", str(tmp_path)) + monkeypatch.setattr(base_client.http.server, "HTTPServer", _FakeHTTPServer) + # Never open a real browser. + monkeypatch.setattr(base_client.webbrowser, "open", lambda url: True) + return base_client + + +def _fake_jwt() -> str: + import base64 + import json + + payload = base64.urlsafe_b64encode( + json.dumps({"tid": "11111111-2222-3333-4444-555555555555"}).encode() + ).rstrip(b"=") + return f"header.{payload.decode('ascii')}.signature" + + +def test_the_shared_core_sign_in_notice_never_touches_stdout( + monkeypatch, tmp_path +) -> None: + """stdout is the MCP JSON-RPC transport. + + A bare line printed there injects non-protocol text into the stream and + corrupts the session for every MCP server built on this shared core, not + just this one. + """ + base_client = _fake_msal(monkeypatch, tmp_path, interactive=True) + + captured_out, captured_err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(captured_out), contextlib.redirect_stderr( + captured_err + ): + token = base_client.acquire_token_msal_interactive() + + assert token == _fake_jwt() + assert captured_out.getvalue() == "", "the sign-in notice reached stdout" + # The cue is preserved for the maker, on the safe stream. + assert "Opening browser" in captured_err.getvalue() + + +def test_a_silent_sign_in_writes_nothing_at_all(monkeypatch, tmp_path) -> None: + base_client = _fake_msal(monkeypatch, tmp_path, interactive=False) + + captured_out, captured_err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(captured_out), contextlib.redirect_stderr( + captured_err + ): + base_client.acquire_token_msal_interactive() + + assert captured_out.getvalue() == "" + assert captured_err.getvalue() == "" + + +def test_constructing_the_authoring_client_writes_nothing_to_stdout( + monkeypatch, tmp_path +) -> None: + """End-to-end: the server's own construction path stays stdout-clean.""" + _fake_msal(monkeypatch, tmp_path, interactive=True) + monkeypatch.delenv("AGENTCONFIG_ACCESS_TOKEN", raising=False) + monkeypatch.delenv("AGENTCONFIG_ACCESS_TOKEN_FILE", raising=False) + monkeypatch.setenv( + "ORG_ANNOUNCEMENTS_BASE_URL", "https://substrate.office.com/weveb2/api/v1.1" + ) + + captured_out = io.StringIO() + + async def run() -> Any: + with contextlib.redirect_stdout(captured_out): + return await org_server.get_client() + + client = asyncio.run(run()) + + assert isinstance(client, org_client.OrgAnnouncementsClient) + assert captured_out.getvalue() == "" + + +def test_no_module_in_the_server_writes_to_stdout_at_import() -> None: + """A bare print at import corrupts the transport before any tool runs.""" + source_dir = ( + REPO_ROOT + / "solutions" + / "ess-maker-skills" + / "src" + / "mcp" + / "agentconfig_org_announcements" + ) + core_dir = ( + REPO_ROOT + / "solutions" + / "ess-maker-skills" + / "src" + / "mcp" + / "agentconfig_core" + ) + + offenders: list[str] = [] + for path in sorted(source_dir.glob("*.py")) + sorted(core_dir.glob("*.py")): + for number, line in enumerate( + path.read_text(encoding="utf-8").splitlines(), 1 + ): + stripped = line.strip() + if not stripped.startswith("print("): + continue + # A print is only acceptable when it is explicitly routed to stderr. + window = "\n".join( + path.read_text(encoding="utf-8").splitlines()[number - 1 : number + 5] + ) + if "file=sys.stderr" not in window: + offenders.append(f"{path.name}:{number}: {stripped}") + + assert not offenders, "print() to stdout in an MCP server module: " + "; ".join( + offenders + ) + + +def test_httpx_logging_stays_off_the_transport() -> None: + """httpx logs requests at INFO; the URL would carry the tenant endpoint.""" + import logging + + assert logging.getLogger("httpx").level >= logging.WARNING + assert logging.getLogger("httpcore").level >= logging.WARNING diff --git a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py new file mode 100644 index 000000000..5957140be --- /dev/null +++ b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py @@ -0,0 +1,2131 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Protocol-level tests for the Org Announcements MCP App and its tools.""" + +from __future__ import annotations + +import asyncio +import copy +import json +import sys +from contextlib import asynccontextmanager +from datetime import datetime, timezone +from pathlib import Path +from typing import Any + +import httpx +import pytest +from mcp.server.fastmcp.exceptions import ToolError +from portalocker.exceptions import LockException + + +REPO_ROOT = Path(__file__).parents[3] +ORG_ANNOUNCEMENTS_DIR = ( + REPO_ROOT + / "solutions" + / "ess-maker-skills" + / "src" + / "mcp" + / "agentconfig_org_announcements" +) +# Sibling MCP servers share the top-level names ``client``/``server``, so the +# modules are loaded through the shared isolated importer rather than by a plain +# ``import`` off ``sys.path``. See tests/mcp/_mcp_modules.py. +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from _mcp_modules import load_org_announcements_modules # noqa: E402 + +_ORG_MODULES = load_org_announcements_modules() +org_server = _ORG_MODULES["server"] +org_client = _ORG_MODULES["client"] +GraphDirectoryError = _ORG_MODULES["graph_directory_client"].GraphDirectoryError + + +RESOURCE_URI = "ui://widget/org-announcements/OrgAnnouncements.html" +TENANT_ID = "11111111-2222-3333-4444-555555555555" +OBJECT_ID = "00000000-0000-0000-0000-000000003333" +TITLE_ID = "title-1" +NOW = datetime(2026, 9, 4, 12, 0, tzinfo=timezone.utc) + +MODEL_VISIBLE_TOOLS = { + "open_org_announcements", "search_audience_groups", + "list_agent_configs", "search_agents", +} +APP_ONLY_TOOLS = {"save_bulletin", "transition_bulletin", "duplicate_bulletin"} + + +def _config( + bulletin_id: str = "bulletin-1", + *, + status: str = "draft", + audience: list[str] | None = None, + end_date: str | None = None, +) -> dict[str, Any]: + return { + "titleId": TITLE_ID, + "bulletin": { + "id": bulletin_id, + "type": "standard", + "priority": 1, + "title": "Quarterly update", + "description": "Read this", + "startDate": "2026-09-01T00:00:00.000Z", + **({"endDate": end_date} if end_date else {}), + }, + "audience": audience if audience is not None else ["g1"], + "status": status, + "createdBy": "admin@contoso.com", + "createdOn": "2026-08-01T12:00:00.000Z", + "modifiedDate": "2026-09-02T12:00:00.000Z", + } + + +class _FakeClient: + """Records every authoring request the server issues. + + Mirrors ``OrgAnnouncementsClient``'s *post-unwrap* contract: the real client + validates and unwraps the ``EssBulletinSaveResult`` envelope, so what the + server sees is a canonical config. Envelope handling itself is covered + against the wire in ``test_authoring_client.py``. + """ + + def __init__( + self, + *, + items: list[dict[str, Any]] | None = None, + get_result: dict[str, Any] | None = None, + save_error: Exception | None = None, + get_error: Exception | None = None, + list_error: Exception | None = None, + transition_error: Exception | None = None, + ) -> None: + self.tenant_id = TENANT_ID + self.object_id = OBJECT_ID + self.title_ids: list[str] = [] + self.items = items if items is not None else [_config()] + self.get_result = get_result if get_result is not None else _config() + self.save_error = save_error + self.get_error = get_error + self.list_error = list_error + self.transition_error = transition_error + self.saves: list[dict[str, Any]] = [] + self.transitions: list[tuple[str, str]] = [] + self.gets: list[str] = [] + self.lists = 0 + + async def list_bulletins(self, title_id: str) -> list[dict[str, Any]]: + self.title_ids.append(title_id) + self.lists += 1 + if self.list_error is not None: + raise self.list_error + return list(self.items) + + async def get_bulletin(self, title_id: str, bulletin_id: str) -> dict[str, Any]: + self.title_ids.append(title_id) + self.gets.append(bulletin_id) + if self.get_error is not None: + raise self.get_error + return self.get_result + + async def save_bulletin(self, title_id: str, payload: dict[str, Any]) -> dict[str, Any]: + self.title_ids.append(title_id) + self.saves.append(payload) + if self.save_error is not None: + raise self.save_error + return _config(payload.get("id") or "created-1", status=payload["status"]) + + async def transition_bulletin( + self, title_id: str, bulletin_id: str, status: str + ) -> dict[str, Any]: + self.title_ids.append(title_id) + self.transitions.append((bulletin_id, status)) + if self.transition_error is not None: + raise self.transition_error + return _config(bulletin_id, status=status) + + +class _StatefulFakeClient(_FakeClient): + """A fake that actually stores records and applies transitions server-side. + + Needed to prove lifecycle preservation for real. A stateless fake can only + show that the *client* sent no content; it cannot show that content + survived, because there is nothing holding the content. This one keeps a + canonical store keyed by ID and mutates only ``status`` on a transition — + exactly what WeveNova's ``SaveAsync`` does — so a round trip that dropped + content would be visible. + """ + + def __init__(self, store: dict[str, dict[str, Any]]) -> None: + super().__init__(items=list(store.values())) + self.store = store + + async def list_bulletins(self, title_id: str) -> list[dict[str, Any]]: + self.title_ids.append(title_id) + self.lists += 1 + return [copy.deepcopy(config) for config in self.store.values()] + + async def get_bulletin(self, title_id: str, bulletin_id: str) -> dict[str, Any]: + self.title_ids.append(title_id) + self.gets.append(bulletin_id) + if bulletin_id not in self.store: + raise org_client.AgentConfigApiError("missing", http_status=404) + return copy.deepcopy(self.store[bulletin_id]) + + async def transition_bulletin( + self, title_id: str, bulletin_id: str, status: str + ) -> dict[str, Any]: + self.title_ids.append(title_id) + self.transitions.append((bulletin_id, status)) + if bulletin_id not in self.store: + raise org_client.AgentConfigApiError("missing", http_status=404) + # Only the status changes; authored content and audience are untouched, + # which is the behavior under test. + self.store[bulletin_id]["status"] = status + return copy.deepcopy(self.store[bulletin_id]) + + +class _FakeGraphClient: + def __init__( + self, + *, + resolved: dict[str, dict[str, Any]] | None = None, + search_groups_result: list[dict[str, Any]] | None = None, + error: Exception | None = None, + ) -> None: + self.resolved = ( + resolved + if resolved is not None + else { + "g1": { + "id": "g1", + "displayName": "Group One", + "mail": None, + "isValid": True, + } + } + ) + self.search_groups_result = search_groups_result or [] + self.error = error + self.resolve_calls: list[list[str]] = [] + self.search_calls: list[str] = [] + self.tenant_ids: list[str] = [] + self.object_ids: list[str] = [] + + async def resolve_groups(self, group_ids) -> dict[str, dict[str, Any]]: + ids = list(group_ids) + self.resolve_calls.append(ids) + if self.error is not None: + raise self.error + return {k: v for k, v in self.resolved.items() if k in ids} + + async def search_groups(self, query: str) -> dict[str, Any]: + self.search_calls.append(query) + if self.error is not None: + raise self.error + return { + "groups": self.search_groups_result, + "exhausted": True, + "pagesExamined": 1, + } + + +@pytest.fixture +def fake_clients(monkeypatch): + def install(client=None, graph=None): + client = client if client is not None else _FakeClient() + graph = graph if graph is not None else _FakeGraphClient() + # get_client is a coroutine: the real one constructs the authoring + # client off the event loop, so the fake must be awaitable too. + async def _get_client(): + return client + + @asynccontextmanager + async def _get_graph_client(tenant_id, object_id): + graph.tenant_ids.append(tenant_id) + graph.object_ids.append(object_id) + yield graph + + monkeypatch.setattr(org_server, "get_client", _get_client) + monkeypatch.setattr(org_server, "get_graph_client", _get_graph_client) + monkeypatch.setattr(org_server, "_now", lambda: NOW) + return client, graph + + return install + + +def _call(tool: str, arguments: dict[str, Any], *, include_scope: bool = True) -> Any: + if include_scope and tool != "search_audience_groups": + arguments = {"titleId": TITLE_ID, **arguments} + async def run() -> Any: + return await org_server.mcp.call_tool(tool, arguments) + + return asyncio.run(run()) + + +def _structured(result: Any) -> dict[str, Any]: + # FastMCP returns (content, structured) for a CallToolResult-returning tool. + if isinstance(result, tuple): + return result[1] + return result.structuredContent + + +# -------------------------------------------------------------------------- +# Resource and tool metadata +# -------------------------------------------------------------------------- + + +def test_widget_origin_uses_the_production_fallback() -> None: + assert ( + org_server.DEFAULT_WIDGET_ORIGIN + == "https://workforceinsights.m365.cloud.microsoft" + ) + + +def test_the_mcp_app_resource_is_registered_with_the_widget_profile() -> None: + async def run(): + return await org_server.mcp.list_resources() + + resources = asyncio.run(run()) + matching = [ + resource for resource in resources if str(resource.uri) == RESOURCE_URI + ] + + assert len(matching) == 1 + assert matching[0].mimeType == org_server.WIDGET_MIME_TYPE + + +def test_the_resource_shell_loads_the_hosted_bundle_from_the_origin() -> None: + shell = org_server.org_announcements_widget() + + assert ( + f'src="{org_server.WIDGET_ORIGIN}/mcp-widget/org-announcements/widget.js"' + in shell + ) + + +def test_widget_origin_override_is_validated() -> None: + assert ( + org_server._resolve_widget_origin("https://localhost:4200") + == "https://localhost:4200" + ) + for bad in [ + "http://localhost:4200", + "https://user:pw@example.com", + "https://example.com/path", + "https://example.com?query=1", + "https://example.com#frag", + ]: + with pytest.raises(ValueError): + org_server._resolve_widget_origin(bad) + + +def test_the_opener_is_read_only_and_visible_to_model_and_app() -> None: + async def run(): + return await org_server.mcp.list_tools() + + tools = {tool.name: tool for tool in asyncio.run(run())} + opener = tools["open_org_announcements"] + + assert opener.annotations.readOnlyHint is True + assert opener.annotations.destructiveHint is False + assert opener.meta["ui"]["visibility"] == ["model", "app"] + assert opener.meta["ui"]["resourceUri"] == RESOURCE_URI + + +def test_mutation_tools_are_not_model_visible() -> None: + async def run(): + return await org_server.mcp.list_tools() + + tools = {tool.name: tool for tool in asyncio.run(run())} + + assert APP_ONLY_TOOLS <= set(tools) + for name in APP_ONLY_TOOLS: + visibility = tools[name].meta["ui"]["visibility"] + assert visibility == ["app"], f"{name} must not be model-visible" + assert tools[name].annotations.readOnlyHint is False + + +def test_search_is_read_only_and_visible_to_both_surfaces() -> None: + async def run(): + return await org_server.mcp.list_tools() + + tools = {tool.name: tool for tool in asyncio.run(run())} + search = tools["search_audience_groups"] + + assert search.annotations.readOnlyHint is True + assert set(search.meta["ui"]["visibility"]) == {"model", "app"} + + +def test_announcement_tools_require_title_id_but_directory_search_does_not() -> None: + + async def run(): + return await org_server.mcp.list_tools() + + for tool in asyncio.run(run()): + properties = tool.inputSchema.get("properties", {}) + assert "tenantId" not in properties, tool.name + if tool.name == "search_audience_groups": + assert set(properties) == {"query"} + elif tool.name == "list_agent_configs": + assert properties == {} + elif tool.name == "search_agents": + assert set(properties) == {"searchString"} + else: + assert properties["titleId"]["type"] == "string" + assert "titleId" in tool.inputSchema["required"] + + +def test_the_server_instructions_disclose_agent_scope() -> None: + instructions = org_server.mcp.instructions.lower() + + assert "authenticated tenant" in instructions + assert "required titleid" in instructions + assert "per tenant and agent" in instructions + + +def test_opener_metadata_exposes_only_normal_create_and_edit_modes(): + tools = asyncio.run(org_server.mcp.list_tools()) + opener = next(tool for tool in tools if tool.name == "open_org_announcements") + variants = opener.inputSchema["properties"]["mode"]["anyOf"] + assert next(variant["enum"] for variant in variants if "enum" in variant) == ["create", "edit"] + assert set(opener.inputSchema["required"]) == {"titleId", "view"} + assert "Manager takes no editor arguments" in opener.description + + +# -------------------------------------------------------------------------- +# Opener behavior +# -------------------------------------------------------------------------- + + +def test_manager_open_returns_canonical_manager_state(fake_clients) -> None: + client, _ = fake_clients( + _FakeClient( + items=[ + _config("a", status="draft"), + _config("b", status="retired"), + ] + ) + ) + + payload = _structured(_call("open_org_announcements", {"view": "manager"})) + + assert payload["view"] == "manager" + assert payload["workingSetCount"] == 1 + assert payload["archivedTruncated"] is False + assert [item["config"]["bulletin"]["id"] for item in payload["items"]] == [ + "a", + "b", + ] + assert client.saves == [] + + +def test_manager_open_message_discloses_agent_scope(fake_clients) -> None: + fake_clients() + + result = _call("open_org_announcements", {"view": "manager"}) + text = result[0][0].text if isinstance(result, tuple) else result.content[0].text + + assert "selected ESS agent" in text + + +def test_empty_create_returns_the_editor_defaults_without_writing( + fake_clients, +) -> None: + client, _ = fake_clients() + + payload = _structured( + _call("open_org_announcements", {"view": "editor", "mode": "create"}) + ) + + assert payload["view"] == "editor" + assert payload["mode"] == "create" + assert payload["config"] is None + assert "id" not in payload["draft"] + assert payload["draft"]["type"] == "standard" + assert payload["draft"]["standardPriority"] == 1 + assert payload["draft"]["audience"] == [] + assert client.saves == [] + assert client.transitions == [] + + +def test_pre_hydrated_create_overlays_the_suggestion_and_writes_nothing( + fake_clients, +) -> None: + client, graph = fake_clients( + graph=_FakeGraphClient( + resolved={ + "g1": { + "id": "g1", + "displayName": "Group One", + "mail": "one@contoso.com", + "isValid": True, + } + } + ) + ) + + payload = _structured( + _call( + "open_org_announcements", + { + "view": "editor", + "mode": "create", + "suggestedDraft": { + "title": "Benefits enrollment", + "priority": 0, + "audience": ["g1"], + "secondaryAction": { + "actionType": "copilotChat", + "label": "Ask", + "prompt": "Explain benefits", + }, + }, + }, + ) + ) + + draft = payload["draft"] + assert draft["title"] == "Benefits enrollment" + assert draft["standardPriority"] == 0 + assert draft["standardSecondaryAction"]["label"] == "Ask" + assert draft["audience"] == [ + { + "id": "g1", + "displayName": "Group One", + "mail": "one@contoso.com", + "isValid": True, + } + ] + # Untouched fields still hold the empty-editor defaults. + assert draft["description"] == "" + assert draft["startDate"] == "" + assert draft["endDate"] == "" + assert draft["primaryAction"] is None + assert client.saves == [] + + +def test_a_suggested_draft_carrying_canonical_metadata_is_rejected( + fake_clients, +) -> None: + """A draft that names a bulletin is rejected before the tool body runs.""" + client, _ = fake_clients() + + with pytest.raises(ToolError): + _call( + "open_org_announcements", + { + "view": "editor", + "mode": "create", + "suggestedDraft": {"title": "x", "id": "bulletin-1"}, + }, + ) + + assert client.saves == [] + + +def test_the_opener_schema_forbids_canonical_draft_fields() -> None: + async def run(): + return await org_server.mcp.list_tools() + + tools = {tool.name: tool for tool in asyncio.run(run())} + schema = tools["open_org_announcements"].inputSchema + suggested = schema["$defs"]["SuggestedBulletinDraft"] + + assert suggested["additionalProperties"] is False + assert set(suggested["properties"]) == { + "type", + "priority", + "title", + "description", + "primaryAction", + "secondaryAction", + "startDate", + "endDate", + "audience", + } + + +def test_edit_open_loads_the_canonical_record(fake_clients) -> None: + client, _ = fake_clients( + _FakeClient(get_result=_config("bulletin-1", status="published")) + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "bulletin-1"}, + ) + ) + + assert client.gets == ["bulletin-1"] + assert payload["mode"] == "edit" + assert payload["config"]["bulletin"]["id"] == "bulletin-1" + assert payload["draft"]["id"] == "bulletin-1" + assert payload["draft"]["startDate"] == "2026-09-01T00:00:00.000Z" + + +def test_editing_an_expired_announcement_preserves_its_schedule( + fake_clients, +) -> None: + fake_clients( + _FakeClient( + get_result=_config( + "bulletin-1", status="published", end_date="2026-08-01T00:00:00.000Z" + ) + ) + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "bulletin-1"}, + ) + ) + + assert payload["mode"] == "edit" + assert payload["draft"]["startDate"] == "2026-09-01T00:00:00.000Z" + assert payload["draft"]["endDate"] == "2026-08-01T00:00:00.000Z" + # Canonical state keeps its stored schedule. + assert payload["config"]["bulletin"]["endDate"] == "2026-08-01T00:00:00.000Z" + + +@pytest.mark.parametrize( + "arguments", + [ + {"view": "manager", "mode": "create"}, + {"view": "manager", "bulletinId": "b"}, + {"view": "manager", "suggestedDraft": {"title": "private"}}, + {"view": "editor"}, + {"view": "editor", "mode": "create", "bulletinId": "b"}, + {"view": "editor", "mode": "edit"}, + {"view": "editor", "mode": "edit", "bulletinId": ""}, + {"view": "editor", "mode": "edit", "bulletinId": " padded "}, + {"view": "editor", "mode": "republish", "bulletinId": "b"}, + { + "view": "editor", + "mode": "edit", + "bulletinId": "b", + "suggestedDraft": {"title": "x"}, + }, + ], +) +def test_incoherent_opener_arguments_are_tool_errors_before_any_io( + fake_clients, arguments +) -> None: + client, directory = fake_clients() + with pytest.raises(ToolError): + _call("open_org_announcements", arguments) + assert client.gets == [] + assert client.lists == 0 + assert directory.tenant_ids == [] + + +def test_open_errors_preserve_the_scoped_request_for_retry( + fake_clients, +) -> None: + fake_clients( + _FakeClient(get_error=org_client.AgentConfigApiError("nope", http_status=404)) + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "secret-id"}, + ) + ) + + assert payload["view"] == "error" + assert payload["code"] == "NotFound" + assert payload["request"] == { + "titleId": TITLE_ID, "view": "editor", "mode": "edit", "bulletinId": "secret-id" + } + assert payload["tenantId"] == TENANT_ID + assert payload["titleId"] == TITLE_ID + + +def test_audience_metadata_failure_returns_audience_metadata_unavailable( + fake_clients, +) -> None: + fake_clients( + graph=_FakeGraphClient( + error=GraphDirectoryError( + "graph down", code="SearchUnavailable", retryable=True + ) + ) + ) + + payload = _structured( + _call( + "open_org_announcements", + { + "view": "editor", + "mode": "create", + "suggestedDraft": {"audience": ["g1"]}, + }, + ) + ) + + assert payload["view"] == "error" + assert payload["code"] == "AudienceMetadataUnavailable" + assert payload["retryable"] is True + + +def test_saved_audiences_that_cannot_be_resolved_stay_present_but_invalid( + fake_clients, +) -> None: + fake_clients( + _FakeClient( + get_result=_config("bulletin-1", audience=["g1", "missing", "g1"]) + ), + _FakeGraphClient( + resolved={ + "g1": { + "id": "g1", + "displayName": "Group One", + "mail": None, + "isValid": True, + } + } + ), + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "bulletin-1"}, + ) + ) + + audience = payload["draft"]["audience"] + assert [group["id"] for group in audience] == ["g1", "missing", "g1"] + assert audience[1]["isValid"] is False + assert audience[1]["displayName"] != "missing" + assert payload["config"]["audience"] == ["g1", "missing", "g1"] + + +@pytest.mark.parametrize( + ("http_status", "expected_code"), + [ + (401, "AuthenticationRequired"), + (403, "AuthorizationDenied"), + (404, "NotFound"), + (405, "FeatureUnavailable"), + (503, "NetworkError"), + ], +) +def test_backend_failures_map_to_discriminated_open_errors( + fake_clients, http_status, expected_code +) -> None: + fake_clients( + _FakeClient( + get_error=org_client.AgentConfigApiError( + "boom", http_status=http_status + ) + ) + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "b"}, + ) + ) + + assert payload["code"] == expected_code + + +@pytest.mark.parametrize("backend_code", ["FeatureDisabled", "FeatureNotEnabled"]) +def test_a_feature_gated_tenant_reports_feature_unavailable( + fake_clients, backend_code +) -> None: + """A disabled tenant is a recoverable unavailable state, not a denial.""" + fake_clients( + _FakeClient( + get_error=org_client.AgentConfigApiError( + f"{backend_code}: Org announcements are off.", http_status=403 + ) + ) + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "b"}, + ) + ) + + assert payload["code"] == "FeatureUnavailable" + assert payload["retryable"] is False + + +def test_open_transport_failure_is_a_retryable_network_error(fake_clients) -> None: + fake_clients( + _FakeClient( + get_error=httpx.ConnectError( + "down", request=httpx.Request("GET", "https://example.invalid") + ) + ) + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "b"}, + ) + ) + + assert payload["code"] == "NetworkError" + assert payload["retryable"] is True + + +# -------------------------------------------------------------------------- +# save_bulletin +# -------------------------------------------------------------------------- + + +def _save_arguments(**overrides) -> dict[str, Any]: + arguments = { + "bulletin": { + "type": "standard", + "priority": 1, + "title": "Quarterly update", + "description": "Read this", + "startDate": "2026-09-01T00:00:00.000Z", + "endDate": "2026-10-01T23:59:59.999Z", + }, + "audience": ["g1"], + "status": "draft", + } + arguments.update(overrides) + return arguments + + +def test_create_sends_complete_content_without_an_identifier(fake_clients) -> None: + client, _ = fake_clients() + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["status"] == "success" + assert "id" not in client.saves[0] + assert client.saves[0]["bulletin"]["title"] == "Quarterly update" + assert client.saves[0]["audience"] == ["g1"] + assert client.saves[0]["status"] == "draft" + + +def test_update_sends_the_identifier_and_complete_state(fake_clients) -> None: + client, _ = fake_clients() + + _call("save_bulletin", _save_arguments(id="bulletin-1", status="published")) + + assert client.saves[0]["id"] == "bulletin-1" + assert client.saves[0]["status"] == "published" + assert client.saves[0]["audience"] == ["g1"] + + +def test_publish_now_is_expressed_as_a_save_with_the_current_instant( + fake_clients, +) -> None: + """Publish now is a save_bulletin call, not a transition.""" + client, _ = fake_clients() + + _call( + "save_bulletin", + _save_arguments( + id="bulletin-1", + status="published", + bulletin={ + "type": "standard", + "priority": 1, + "title": "Quarterly update", + "description": "Read this", + "startDate": "2026-09-04T12:00:00.000Z", + "endDate": "2026-10-01T23:59:59.999Z", + }, + ), + ) + + assert client.saves[0]["status"] == "published" + assert client.saves[0]["bulletin"]["startDate"] == "2026-09-04T12:00:00.000Z" + assert client.transitions == [] + + +def test_a_draft_with_blank_dates_sends_no_empty_string_to_the_api( + fake_clients, +) -> None: + """End-to-end: the widget's ``""`` sentinel never reaches the wire. + + ``EssBulletinInput.startDate``/``endDate`` are nullable ``DateTimeOffset``. + An empty string fails the backend's model binding, which comes back as an + opaque 400 rather than the field-bound validation error the widget renders — + so an ordinary unscheduled Draft would simply be unsaveable. + """ + client, _ = fake_clients() + + payload = _structured( + _call( + "save_bulletin", + _save_arguments( + bulletin={ + "type": "standard", + "title": "Quarterly update", + "description": "Read this", + "startDate": "", + "endDate": " ", + } + ), + ) + ) + + assert payload["status"] == "success" + sent = client.saves[0] + assert "startDate" not in sent["bulletin"] + assert "endDate" not in sent["bulletin"] + # Asserted against the serialized form too: the wire is what binds. + assert '""' not in json.dumps(sent) + + +def test_a_published_save_with_blank_dates_still_reaches_the_backend( + fake_clients, +) -> None: + """Publish-time completeness stays WeveNova's call, not a contract error.""" + client, _ = fake_clients() + + _call( + "save_bulletin", + _save_arguments( + bulletin={ + "type": "standard", + "title": "Quarterly update", + "description": "Read this", + "startDate": "", + "endDate": "", + }, + status="published", + ), + ) + + # The request was issued rather than rejected locally, so the backend can + # answer with its own structured AudienceRequired/date codes. + assert len(client.saves) == 1 + assert client.saves[0]["status"] == "published" + assert "startDate" not in client.saves[0]["bulletin"] + + +def test_a_duplicate_of_a_dateless_source_sends_no_empty_string( + fake_clients, +) -> None: + """The duplicate path rebuilds the payload from stored content.""" + source = _config("bulletin-1") + source["bulletin"]["startDate"] = "" + source["bulletin"]["endDate"] = "" + client, _ = fake_clients(_FakeClient(get_result=source)) + + _call("duplicate_bulletin", {"id": "bulletin-1"}) + + sent = client.saves[0] + # Duplicate forwards stored content verbatim and so never passes through + # BulletinInput; the sentinel must be stripped on this path explicitly. + assert "startDate" not in sent["bulletin"] + assert "endDate" not in sent["bulletin"] + assert '""' not in json.dumps(sent) + assert sent["status"] == "draft" + # Real content is still copied. + assert sent["bulletin"]["title"] == "Quarterly update" + + +def test_save_returns_the_canonical_item_and_refreshed_manager_state( + fake_clients, +) -> None: + fake_clients() + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["item"]["config"]["bulletin"]["id"] == "created-1" + assert payload["item"]["audienceMetadata"][0]["id"] == "g1" + assert "workingSetCount" in payload["manager"] + assert "archivedTruncated" in payload["manager"] + + +def test_an_indeterminate_create_is_reported_and_not_retried(fake_clients) -> None: + client, _ = fake_clients( + _FakeClient( + save_error=org_client.IndeterminateWriteError( + "may have been created; refresh before retrying" + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["status"] == "failure" + assert payload["errors"][0]["code"] == "IndeterminateWrite" + assert payload["errors"][0]["retryable"] is False + assert len(client.saves) == 1 + + +def test_structured_backend_validation_errors_are_preserved(fake_clients) -> None: + fake_clients( + _FakeClient( + save_error=org_client.AgentConfigApiError( + "AudienceRequired: At least one audience is required.", + http_status=400, + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments(audience=["g1"]))) + + assert payload["errors"][0]["code"] == "AudienceRequired" + assert "At least one audience is required." in payload["errors"][0]["message"] + + +def test_backend_limit_errors_are_preserved(fake_clients) -> None: + fake_clients( + _FakeClient( + save_error=org_client.AgentConfigApiError( + "BulletinLimitExceeded: Too many announcements.", http_status=409 + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["errors"][0]["code"] == "BulletinLimitExceeded" + + +def test_http_200_validation_errors_are_reported_individually(fake_clients) -> None: + """Every ``{code, field, message}`` entry survives to the widget.""" + fake_clients( + _FakeClient( + save_error=org_client.BulletinValidationError( + [ + { + "code": "AudienceRequired", + "field": "audience", + "message": "Pick at least one group.", + }, + { + "code": "TitleRequired", + "field": "title", + "message": "Add a title.", + }, + ] + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["status"] == "failure" + assert [error["code"] for error in payload["errors"]] == [ + "AudienceRequired", + "TitleRequired", + ] + assert [error["field"] for error in payload["errors"]] == [ + "audience", + "title", + ] + assert payload["errors"][1]["message"] == "Add a title." + # Nothing is flattened into a single generic error. + assert all(error["retryable"] is False for error in payload["errors"]) + + +def test_a_server_failure_is_not_reported_as_a_validation_error( + fake_clients, +) -> None: + """A 500 must not surface the core's ``HttpError`` placeholder as a code.""" + fake_clients( + _FakeClient( + save_error=org_client.AgentConfigApiError( + "HttpError: HTTP 500", http_status=500 + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + error = payload["errors"][0] + assert error["code"] == "ServiceError" + assert error["retryable"] is False + # Privacy-safe: no raw backend text and no placeholder code leaks out. + assert "HttpError" not in error["message"] + assert "500" not in error["message"] + + +def test_a_400_without_a_backend_code_is_a_generic_invalid_request( + fake_clients, +) -> None: + fake_clients( + _FakeClient( + save_error=org_client.AgentConfigApiError( + "HttpError: HTTP 400", http_status=400 + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["errors"][0]["code"] == "InvalidRequest" + + +def test_an_unclassified_backend_failure_is_a_stable_service_error( + fake_clients, +) -> None: + fake_clients( + _FakeClient( + save_error=org_client.AgentConfigApiError( + "Maximum retries exceeded: boom", http_status=None + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["errors"][0]["code"] == "ServiceError" + assert "boom" not in payload["errors"][0]["message"] + + +def test_a_feature_code_on_a_server_failure_still_reports_unavailable( + fake_clients, +) -> None: + """A genuine backend feature code wins over the HTTP status.""" + fake_clients( + _FakeClient( + save_error=org_client.AgentConfigApiError( + "FeatureDisabled: not enabled", http_status=500 + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["errors"][0]["code"] == "FeatureUnavailable" + + +# -------------------------------------------------------------------------- +# Committed-write semantics +# -------------------------------------------------------------------------- + + +def test_a_refresh_failure_after_a_committed_save_is_not_retryable( + fake_clients, +) -> None: + """The write landed. Retrying an unkeyed create would duplicate it.""" + client, _ = fake_clients( + _FakeClient( + list_error=org_client.AgentConfigApiError( + "HttpError: HTTP 503", http_status=503 + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["status"] == "failure" + summary = payload["errors"][0] + assert summary["code"] == "CommittedRefreshFailed" + assert summary["retryable"] is False + assert "saved" in summary["message"].lower() + assert "do not repeat the action" in summary["message"].lower() + # The underlying refresh failure is preserved for diagnosis. + assert payload["errors"][1]["code"] == "NetworkError" + # Exactly one write was issued; nothing replayed it. + assert len(client.saves) == 1 + + +def test_a_graph_failure_after_a_committed_save_is_not_retryable( + fake_clients, +) -> None: + client, _ = fake_clients( + graph=_FakeGraphClient( + error=GraphDirectoryError( + "graph down", code="SearchUnavailable", retryable=True + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["errors"][0]["code"] == "CommittedRefreshFailed" + assert payload["errors"][0]["retryable"] is False + assert len(client.saves) == 1 + + +def test_a_refresh_failure_after_a_committed_duplicate_is_not_retryable( + fake_clients, +) -> None: + """Duplicate is an unkeyed create; a retry would make a second copy.""" + client, _ = fake_clients( + _FakeClient( + list_error=org_client.AgentConfigApiError( + "HttpError: HTTP 503", http_status=503 + ) + ) + ) + + payload = _structured(_call("duplicate_bulletin", {"id": "bulletin-1"})) + + assert payload["errors"][0]["code"] == "CommittedRefreshFailed" + assert payload["errors"][0]["retryable"] is False + assert len(client.saves) == 1 + + +def test_a_refresh_failure_after_a_committed_transition_is_not_retryable( + fake_clients, +) -> None: + client, _ = fake_clients( + _FakeClient( + list_error=org_client.AgentConfigApiError( + "HttpError: HTTP 503", http_status=503 + ) + ) + ) + + payload = _structured( + _call("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}) + ) + + assert payload["errors"][0]["code"] == "CommittedRefreshFailed" + assert payload["errors"][0]["retryable"] is False + assert client.transitions == [("bulletin-1", "retired")] + + +def test_a_failed_write_stays_a_normal_retryable_failure(fake_clients) -> None: + """Nothing committed, so the widget may legitimately offer a retry.""" + fake_clients( + _FakeClient( + save_error=org_client.AgentConfigApiError( + "HttpError: HTTP 503", http_status=503 + ) + ) + ) + + payload = _structured(_call("save_bulletin", _save_arguments())) + + assert payload["errors"][0]["code"] == "NetworkError" + assert payload["errors"][0]["retryable"] is True + + +def test_save_rejects_an_unknown_content_field(fake_clients) -> None: + client, _ = fake_clients() + + payload = _structured( + _call( + "save_bulletin", + _save_arguments( + bulletin={ + "type": "standard", + "title": "t", + "description": "d", + "id": "sneaky", + } + ), + ) + ) + + assert payload["status"] == "failure" + assert payload["errors"][0]["code"] == "InvalidRequest" + assert client.saves == [] + + +# -------------------------------------------------------------------------- +# transition_bulletin +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("transition", "status"), + [ + ("archive", "retired"), + ("unarchive", "draft"), + ("moveToDraft", "draft"), + ("delete", "deleted"), + ], +) +def test_transitions_map_to_minimal_status_payloads( + fake_clients, transition, status +) -> None: + client, _ = fake_clients() + + payload = _structured( + _call("transition_bulletin", {"id": "bulletin-1", "transition": transition}) + ) + + assert client.transitions == [("bulletin-1", status)] + assert client.saves == [] + assert payload["status"] == "success" + assert "manager" in payload + # The canonical changed row is included alongside the manager state. It is + # additive, so a host that strips unknown fields still gets the refresh. + assert payload["item"]["config"]["bulletin"]["id"] == "bulletin-1" + assert payload["item"]["config"]["status"] == status + + +def test_transition_success_keeps_the_manager_shape_intact(fake_clients) -> None: + """The added item must not disturb the manager contract Vorpal reads.""" + fake_clients(_FakeClient(items=[_config("bulletin-1", audience=["g1"])])) + + payload = _structured( + _call("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}) + ) + + manager = payload["manager"] + assert set(manager) == { + "tenantId", "titleId", "items", "workingSetCount", "archivedTruncated" + } + assert set(manager["items"][0]) == {"config", "audienceMetadata"} + + +def test_publish_now_is_not_a_transition_operation(fake_clients) -> None: + """Publish now must route through save_bulletin, never a transition.""" + client, _ = fake_clients() + + with pytest.raises(ToolError): + _call( + "transition_bulletin", + {"id": "bulletin-1", "transition": "publishNow"}, + ) + + assert client.transitions == [] + assert client.saves == [] + + +def test_a_legacy_publish_now_is_rejected_by_host_validation(fake_clients) -> None: + """Documents where a legacy Vorpal build's ``publishNow`` is stopped. + + KNOWN LIMITATION — host-level, not tool-level. FastMCP validates arguments + against the schema derived from the tool signature *before* the tool body + runs, so a legacy ``publishNow`` surfaces as a protocol ``ToolError``, not + as this server's structured ``InvalidRequest`` result. Converting it would + mean widening ``transition`` from the four-value enum to a free string, + which would advertise ``publishNow`` as acceptable in the production schema + and re-open the concurrent-edit race that removing it closed. + + The mitigation is the rollout prerequisite: ship a Vorpal build that routes + publish-now through ``save_bulletin``. This test pins the current boundary + so the behavior is a known, tested contract rather than a surprise, and so + the day a supported FastMCP pre-validation hook exists the change is + visible here. + """ + client, _ = fake_clients() + + with pytest.raises(ToolError) as caught: + _call( + "transition_bulletin", + {"id": "bulletin-1", "transition": "publishNow"}, + ) + + # The rejection names the field and the permitted values, so the failure is + # at least diagnosable from the protocol error text. + message = str(caught.value) + assert "transition" in message + assert "publishNow" in message + for supported in ("archive", "unarchive", "moveToDraft", "delete"): + assert supported in message + # Nothing reached the backend. + assert client.transitions == [] + assert client.saves == [] + + +def test_title_id_reaches_the_client_but_never_the_save_body( + fake_clients, +) -> None: + client, _ = fake_clients() + + payload = _structured( + _call("save_bulletin", {**_save_arguments(), "titleId": TITLE_ID}) + ) + + assert payload["status"] == "success" + assert len(client.saves) == 1 + assert client.title_ids == [TITLE_ID, TITLE_ID] + assert payload["tenantId"] == TENANT_ID + assert payload["titleId"] == TITLE_ID + assert "titleId" not in client.saves[0] + assert "titleId" not in client.saves[0]["bulletin"] + + +def test_the_transition_schema_lists_only_the_supported_operations() -> None: + async def run(): + return await org_server.mcp.list_tools() + + tools = {tool.name: tool for tool in asyncio.run(run())} + transition = tools["transition_bulletin"].inputSchema["properties"][ + "transition" + ] + + assert set(transition["enum"]) == { + "archive", + "unarchive", + "moveToDraft", + "delete", + } + assert "publishNow" not in transition["enum"] + + +def test_archive_then_unarchive_preserves_content_and_audience( + fake_clients, +) -> None: + """Content and audience survive a full Archive -> Unarchive round trip. + + Proven against a *stateful* fake that stores the canonical record and, like + WeveNova's ``SaveAsync``, mutates only ``status`` on a transition. The + earlier version of this test asserted only that the client sent no content, + which cannot distinguish "the backend preserved it" from "there was never + any content to lose". + + Limitation: this simulates the documented backend behavior. That WeveNova + genuinely preserves content through these transitions is a live check in the + target ring — see the backend alignment gate in the dev spec. + """ + original = _config( + "bulletin-1", status="published", audience=["g1", "g2", "g1"] + ) + original["bulletin"]["title"] = "Benefits enrollment" + original["bulletin"]["description"] = "Enroll before Friday." + store = {"bulletin-1": copy.deepcopy(original)} + client, _ = fake_clients(_StatefulFakeClient(store)) + + archived = _structured( + _call("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}) + ) + unarchived = _structured( + _call("transition_bulletin", {"id": "bulletin-1", "transition": "unarchive"}) + ) + + assert client.transitions == [ + ("bulletin-1", "retired"), + ("bulletin-1", "draft"), + ] + # No client-side read/merge/write: the payload carried only {id, status}. + assert client.saves == [] + assert client.gets == [] + + assert archived["item"]["config"]["status"] == "retired" + assert unarchived["item"]["config"]["status"] == "draft" + + # Everything except status is byte-identical to the original record. + restored = store["bulletin-1"] + assert restored["bulletin"] == original["bulletin"] + # Order and multiplicity are preserved exactly, duplicates included. + assert restored["audience"] == ["g1", "g2", "g1"] + + +def test_published_to_draft_preserves_content_through_a_stateful_backend( + fake_clients, +) -> None: + original = _config("bulletin-1", status="published", audience=["g1"]) + original["bulletin"]["title"] = "Quarterly all-hands" + store = {"bulletin-1": copy.deepcopy(original)} + client, _ = fake_clients(_StatefulFakeClient(store)) + + payload = _structured( + _call( + "transition_bulletin", {"id": "bulletin-1", "transition": "moveToDraft"} + ) + ) + + assert client.transitions == [("bulletin-1", "draft")] + assert client.saves == [] + assert payload["item"]["config"]["status"] == "draft" + assert store["bulletin-1"]["bulletin"] == original["bulletin"] + assert store["bulletin-1"]["audience"] == ["g1"] + + +def test_deleted_rows_are_excluded_from_the_manager(fake_clients) -> None: + fake_clients( + _FakeClient( + items=[ + _config("keep-1", status="draft"), + _config("gone-1", status="deleted"), + ] + ) + ) + + payload = _structured(_call("open_org_announcements", {"view": "manager"})) + + assert [item["config"]["bulletin"]["id"] for item in payload["items"]] == [ + "keep-1" + ] + assert payload["workingSetCount"] == 1 + + +# -------------------------------------------------------------------------- +# duplicate_bulletin +# -------------------------------------------------------------------------- + + +def test_duplicate_strips_identity_and_audit_fields_and_creates_a_draft( + fake_clients, +) -> None: + source = _config("bulletin-1", status="published", audience=["g1", "g2"]) + source["bulletin"]["modifiedDate"] = "2026-09-02T12:00:00.000Z" + client, _ = fake_clients(_FakeClient(get_result=source)) + + payload = _structured(_call("duplicate_bulletin", {"id": "bulletin-1"})) + + assert client.gets == ["bulletin-1"] + saved = client.saves[0] + assert "id" not in saved + assert "id" not in saved["bulletin"] + assert "modifiedDate" not in saved["bulletin"] + assert saved["status"] == "draft" + assert saved["audience"] == ["g1", "g2"] + assert saved["bulletin"]["title"] == "Quarterly update" + assert payload["status"] == "success" + + +def test_duplicate_of_a_missing_source_is_not_a_create(fake_clients) -> None: + client, _ = fake_clients( + _FakeClient( + get_error=org_client.AgentConfigApiError("gone", http_status=404) + ) + ) + + payload = _structured(_call("duplicate_bulletin", {"id": "bulletin-1"})) + + assert payload["status"] == "failure" + assert payload["errors"][0]["code"] == "NotFound" + assert client.saves == [] + + +def test_duplicate_reports_an_indeterminate_write(fake_clients) -> None: + client, _ = fake_clients( + _FakeClient( + save_error=org_client.IndeterminateWriteError( + "may have been created; refresh before retrying" + ) + ) + ) + + payload = _structured(_call("duplicate_bulletin", {"id": "bulletin-1"})) + + assert payload["errors"][0]["code"] == "IndeterminateWrite" + assert len(client.saves) == 1 + + +# -------------------------------------------------------------------------- +# search_audience_groups +# -------------------------------------------------------------------------- + + +def test_search_returns_eligible_groups(fake_clients) -> None: + _, graph = fake_clients( + graph=_FakeGraphClient( + search_groups_result=[ + { + "id": "g1", + "displayName": "Finance", + "mail": "fin@contoso.com", + "isValid": True, + } + ] + ) + ) + + payload = _structured(_call("search_audience_groups", {"query": "Finance"})) + + assert payload["status"] == "success" + assert payload["groups"][0]["id"] == "g1" + assert graph.search_calls == ["Finance"] + + +def test_search_reports_exhaustion_and_pages_examined(fake_clients) -> None: + """A capped search must be distinguishable from an exhausted one. + + Without these fields the caller cannot tell "there are no more matching + groups" from "the page budget ran out", and would tell the maker a group + does not exist when it simply was not reached. + """ + + class _CappedGraphClient(_FakeGraphClient): + async def search_groups(self, query: str) -> dict[str, Any]: + self.search_calls.append(query) + return {"groups": [], "exhausted": False, "pagesExamined": 3} + + fake_clients(graph=_CappedGraphClient()) + + payload = _structured(_call("search_audience_groups", {"query": "Eng"})) + + assert payload["exhausted"] is False + assert payload["pagesExamined"] == 3 + + +def test_search_reports_an_exhausted_result_set(fake_clients) -> None: + fake_clients(graph=_FakeGraphClient()) + + payload = _structured(_call("search_audience_groups", {"query": "Eng"})) + + assert payload["exhausted"] is True + assert payload["pagesExamined"] == 1 + + +def test_search_failures_return_a_discriminated_failure(fake_clients) -> None: + fake_clients( + graph=_FakeGraphClient( + error=GraphDirectoryError( + "consent required", code="AuthorizationDenied", retryable=False + ) + ) + ) + + payload = _structured(_call("search_audience_groups", {"query": "Finance"})) + + assert payload["status"] == "failure" + assert payload["code"] == "AuthorizationDenied" + assert payload["retryable"] is False + + +def test_search_rejects_an_invalid_query(fake_clients) -> None: + fake_clients(graph=_FakeGraphClient(error=ValueError("query must be non-empty"))) + + payload = _structured(_call("search_audience_groups", {"query": " "})) + + assert payload["status"] == "failure" + assert payload["code"] == "InvalidRequest" + + +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("open_org_announcements", {"view": "manager"}), + ("save_bulletin", _save_arguments()), + ("transition_bulletin", {"id": "a", "transition": "archive"}), + ("duplicate_bulletin", {"id": "a"}), + ], +) +def test_missing_title_is_a_host_error_before_any_client_call(monkeypatch, tool, arguments) -> None: + async def forbidden_client(): + pytest.fail("missing title attempted authentication") + + monkeypatch.setattr(org_server, "get_client", forbidden_client) + with pytest.raises(ToolError, match="titleId"): + _call(tool, arguments, include_scope=False) + + +@pytest.mark.parametrize("title_id", ["", " padded ", "\x01", "x" * 257]) +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("open_org_announcements", {"view": "editor", "mode": "create"}), + ("save_bulletin", _save_arguments()), + ("transition_bulletin", {"id": "a", "transition": "archive"}), + ("duplicate_bulletin", {"id": "a"}), + ], +) +def test_invalid_title_never_acquires_a_client(monkeypatch, title_id, tool, arguments) -> None: + async def forbidden_client(): + pytest.fail("invalid title attempted authentication") + + monkeypatch.setattr(org_server, "get_client", forbidden_client) + if tool == "open_org_announcements": + with pytest.raises(ToolError, match="titleId"): + _call(tool, {**arguments, "titleId": title_id}) + return + payload = _structured(_call(tool, {**arguments, "titleId": title_id})) + assert payload["titleId"] == title_id + assert "tenantId" not in payload + assert "item" not in payload + assert "manager" not in payload + assert payload.get("code") == "InvalidRequest" or payload["errors"][0]["code"] == "InvalidRequest" + + +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("open_org_announcements", {"view": "manager"}), + ("open_org_announcements", {"view": "editor", "mode": "create"}), + ("open_org_announcements", {"view": "editor", "mode": "edit", "bulletinId": "bulletin-1"}), + ("save_bulletin", _save_arguments()), + ("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}), + ("duplicate_bulletin", {"id": "bulletin-1"}), + ], +) +def test_success_envelopes_and_reusable_managers_carry_scope(fake_clients, tool, arguments) -> None: + fake_clients() + payload = _structured(_call(tool, arguments)) + assert payload["titleId"] == TITLE_ID + assert payload["tenantId"] == TENANT_ID + if "manager" in payload: + assert payload["manager"]["tenantId"] == TENANT_ID + assert payload["manager"]["titleId"] == TITLE_ID + if payload.get("config") is not None: + assert payload["config"]["titleId"] == TITLE_ID + if "item" in payload: + assert payload["item"]["config"]["titleId"] == TITLE_ID + + +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("open_org_announcements", {"view": "editor", "mode": "create"}), + ("save_bulletin", _save_arguments()), + ("transition_bulletin", {"id": "a", "transition": "archive"}), + ("duplicate_bulletin", {"id": "a"}), + ], +) +def test_auth_failure_before_tenant_resolution_omits_tenant(monkeypatch, tool, arguments) -> None: + async def unavailable(): + raise org_client.AgentConfigApiError("Sign in", http_status=401) + + monkeypatch.setattr(org_server, "get_client", unavailable) + payload = _structured(_call(tool, arguments)) + assert payload["titleId"] == TITLE_ID + assert "tenantId" not in payload + assert "manager" not in payload + assert "item" not in payload + assert payload.get("code") == "AuthenticationRequired" or payload["errors"][0]["code"] == "AuthenticationRequired" + + +def test_empty_create_acquires_scope_but_never_reads_or_writes(fake_clients, monkeypatch) -> None: + client, graph = fake_clients() + calls = [] + + async def get_client(): + calls.append(True) + return client + + monkeypatch.setattr(org_server, "get_client", get_client) + payload = _structured(_call("open_org_announcements", {"view": "editor", "mode": "create"})) + assert calls == [True] + assert payload["tenantId"] == TENANT_ID + assert payload["config"] is None + assert "id" not in payload["draft"] + assert client.lists == 0 + assert client.gets == client.saves == client.transitions == [] + assert graph.resolve_calls == [] + + +def test_original_suggestion_and_scope_survive_retry_without_logging(fake_clients, caplog) -> None: + _, graph = fake_clients(graph=_FakeGraphClient( + error=GraphDirectoryError("unavailable", code="SearchUnavailable", retryable=True) + )) + request = { + "titleId": TITLE_ID, + "view": "editor", + "mode": "create", + "suggestedDraft": { + "title": "Private retry proposal", + "startDate": "2026-09-12", + "audience": ["private-audience"], + }, + } + failure = _structured(_call("open_org_announcements", request)) + assert failure["request"] == request + assert failure["tenantId"] == TENANT_ID + assert "config" not in failure and "draft" not in failure + assert "Private retry proposal" not in caplog.text + assert "private-audience" not in caplog.text + graph.error = None + retry = _structured(_call("open_org_announcements", failure["request"])) + assert retry["titleId"] == TITLE_ID + assert retry["draft"]["startDate"] == "2026-09-12T00:00:00.000Z" + + +def test_postcommit_refresh_keeps_the_captured_client_and_scope(fake_clients, monkeypatch) -> None: + replacement = _FakeClient() + replacement.tenant_id = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + + async def replacement_client(): + return replacement + + class SwitchingClient(_FakeClient): + async def save_bulletin(self, title_id, payload): + saved = await super().save_bulletin(title_id, payload) + monkeypatch.setattr(org_server, "get_client", replacement_client) + return saved + + original = SwitchingClient() + fake_clients(original) + payload = _structured(_call("save_bulletin", _save_arguments())) + assert payload["tenantId"] == payload["manager"]["tenantId"] == TENANT_ID + assert payload["titleId"] == payload["manager"]["titleId"] == TITLE_ID + assert original.title_ids == [TITLE_ID, TITLE_ID] + assert replacement.lists == 0 + + +def test_caller_cannot_override_the_token_tenant(fake_clients) -> None: + fake_clients() + payload = _structured(_call("open_org_announcements", { + "view": "manager", "tenantId": "arbitrary-tenant" + })) + assert payload["tenantId"] == TENANT_ID + + +@pytest.mark.parametrize("operation", ["manager", "edit", "duplicate", "save", "transition"]) +@pytest.mark.parametrize("response_title", [None, "wrong-agent"]) +def test_wrong_scope_is_rejected_before_audience_hydration( + monkeypatch, fake_clients, operation, response_title +) -> None: + import base64 + + token_payload = base64.urlsafe_b64encode(json.dumps({"tid": TENANT_ID}).encode()).rstrip(b"=") + monkeypatch.setenv("AGENTCONFIG_ACCESS_TOKEN", f"header.{token_payload.decode()}.signature") + monkeypatch.delenv("AGENTCONFIG_ACCESS_TOKEN_FILE", raising=False) + config = _config() + if response_title is None: + config.pop("titleId") + else: + config["titleId"] = response_title + requests = [] + + def handler(request): + requests.append(request) + if operation == "manager": + body = [config] + elif operation in ("save", "transition"): + body = {"id": "bulletin-1", "config": config, "errors": []} + else: + body = config + return httpx.Response(200, json=body) + + client = org_client.OrgAnnouncementsClient(transport=httpx.MockTransport(handler)) + _, graph = fake_clients(client) + tool, arguments = { + "duplicate": ("duplicate_bulletin", {"id": "bulletin-1"}), + "manager": ("open_org_announcements", {"view": "manager"}), + "edit": ("open_org_announcements", { + "view": "editor", "mode": "edit", "bulletinId": "bulletin-1" + }), + "save": ("save_bulletin", {**_save_arguments(), "id": "bulletin-1"}), + "transition": ("transition_bulletin", { + "id": "bulletin-1", "transition": "archive" + }), + }[operation] + payload = _structured(_call(tool, arguments)) + assert payload.get("code") == "ServiceError" or payload["errors"][0]["code"] == "ServiceError" + assert payload["titleId"] == TITLE_ID + assert "config" not in payload and "item" not in payload and "manager" not in payload + assert graph.resolve_calls == [] + assert len(requests) == 1 + asyncio.run(client.aclose()) + + +def test_overlapping_opens_do_not_share_an_active_title(fake_clients) -> None: + class ScopedClient(_FakeClient): + async def list_bulletins(self, title_id): + self.title_ids.append(title_id) + await asyncio.sleep(0) + return [{**_config("shared-id", audience=[]), "titleId": title_id}] + + client = ScopedClient() + fake_clients(client) + + async def run(): + return await asyncio.gather(*( + org_server.mcp.call_tool("open_org_announcements", { + "titleId": title, "view": "manager" + }) + for title in ("first-agent", "second-agent") + )) + + results = asyncio.run(run()) + for title, result in zip(("first-agent", "second-agent"), results): + payload = _structured(result) + assert payload["tenantId"] == TENANT_ID + assert payload["titleId"] == title + assert payload["items"][0]["config"]["titleId"] == title + assert client.title_ids == ["first-agent", "second-agent"] + + +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("open_org_announcements", {"view": "manager"}), + ("open_org_announcements", {"view": "editor", "mode": "create"}), + ("open_org_announcements", { + "view": "editor", "mode": "create", "suggestedDraft": {"audience": ["g1"]} + }), + ("open_org_announcements", { + "view": "editor", "mode": "edit", "bulletinId": "bulletin-1" + }), + ("save_bulletin", _save_arguments()), + ("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}), + ("duplicate_bulletin", {"id": "bulletin-1"}), + ("search_audience_groups", {"query": "finance"}), + ], +) +def test_directory_work_uses_the_captured_authoring_tenant(fake_clients, tool, arguments) -> None: + client, directory = fake_clients() + payload = _structured(_call(tool, arguments)) + assert payload.get("status") != "failure" + assert payload.get("view") != "error" + assert directory.tenant_ids == [client.tenant_id] + assert directory.object_ids == [client.object_id] + if tool == "search_audience_groups": + assert set(payload) == {"status", "groups", "exhausted", "pagesExamined"} + + +def test_post_save_directory_refresh_keeps_the_original_authoring_tenant( + fake_clients, monkeypatch +) -> None: + replacement = _FakeClient() + replacement.tenant_id = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + replacement.object_id = "00000000-0000-0000-0000-000000004444" + + async def replacement_client(): + return replacement + + class SwitchingClient(_FakeClient): + async def save_bulletin(self, title_id, payload): + saved = await super().save_bulletin(title_id, payload) + monkeypatch.setattr(org_server, "get_client", replacement_client) + return saved + + original = SwitchingClient() + _, directory = fake_clients(original) + payload = _structured(_call("save_bulletin", _save_arguments())) + assert payload["tenantId"] == TENANT_ID + assert directory.tenant_ids == [TENANT_ID] + assert directory.object_ids == [OBJECT_ID] + assert replacement.lists == 0 + assert len(directory.resolve_calls) == 2 + + +def test_search_requires_authoring_context_without_changing_its_result_schema( + fake_clients, monkeypatch +) -> None: + _, directory = fake_clients() + + async def unavailable(): + raise org_client.AgentConfigApiError("Sign in", http_status=401) + + monkeypatch.setattr(org_server, "get_client", unavailable) + payload = _structured(_call("search_audience_groups", {"query": "finance"})) + assert payload == { + "status": "failure", "code": "AuthenticationRequired", "retryable": False + } + assert directory.tenant_ids == [] + + +@pytest.mark.parametrize("change", ["tenant", "account"]) +def test_graph_client_replacement_preserves_inflight_users_and_bounds_idle_state( + monkeypatch, change +) -> None: + other_tenant = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" if change == "tenant" else TENANT_ID + other_object = "00000000-0000-0000-0000-000000004444" if change == "account" else OBJECT_ID + actual_type = org_server.GraphDirectoryClient + created = [] + closed = [] + + class TrackingClient(actual_type): + def __init__(self, **kwargs): + super().__init__(**kwargs) + created.append(self) + + async def aclose(self): + closed.append(self) + await super().aclose() + + monkeypatch.setattr(org_server, "GraphDirectoryClient", TrackingClient) + monkeypatch.setattr(org_server, "_graph_client", None) + monkeypatch.setattr(org_server, "_graph_client_users", {}) + + async def run(): + async with org_server.get_graph_client(TENANT_ID, OBJECT_ID) as original: + original._metadata_cache.put("g1", {"id": "g1", "displayName": "tenant A"}) + async with org_server.get_graph_client(TENANT_ID, OBJECT_ID) as same: + assert same is original + async with org_server.get_graph_client(other_tenant, other_object) as replacement: + assert replacement is not original + assert original.tenant_id == TENANT_ID + assert replacement.tenant_id == other_tenant + assert original.object_id == OBJECT_ID + assert replacement.object_id == other_object + assert replacement._metadata_cache.get("g1") is None + replacement._metadata_cache.put("g1", {"id": "g1", "displayName": "tenant B"}) + assert original not in closed + assert original not in closed + assert original._metadata_cache.get("g1")["displayName"] == "tenant A" + assert closed == [original] + async with org_server.get_graph_client(other_tenant, other_object) as reused: + assert reused is replacement + assert reused._metadata_cache.get("g1")["displayName"] == "tenant B" + assert len(created) == 2 + assert org_server._graph_client_users == {} + await replacement.aclose() + + asyncio.run(run()) + + +def test_directory_tenant_binding_does_not_add_mcp_arguments() -> None: + tools = asyncio.run(org_server.mcp.list_tools()) + for tool in tools: + properties = tool.inputSchema["properties"] + assert "tenantId" not in properties + assert "expectedTenantId" not in properties + assert "objectId" not in properties + assert "accountId" not in properties + if tool.name == "search_audience_groups": + assert set(properties) == {"query"} + + +_PRIVATE_CACHE_DETAIL = "/private/msal-cache PRIVATE_CREDENTIAL_DETAIL" + + +def _install_failing_graph_acquisition(monkeypatch, error, *, before_failure=None): + directory = org_server.GraphDirectoryClient(tenant_id=TENANT_ID, object_id=OBJECT_ID) + acquisitions = [] + + def acquire(tenant_id, object_id): + acquisitions.append(tenant_id) + assert object_id == OBJECT_ID + if before_failure is not None: + before_failure() + raise error + + @asynccontextmanager + async def get_graph_client(tenant_id, object_id): + assert tenant_id == TENANT_ID + assert object_id == OBJECT_ID + try: + yield directory + finally: + await directory.aclose() + + monkeypatch.setattr( + _ORG_MODULES["graph_directory_client"], "acquire_graph_token", acquire + ) + monkeypatch.setattr(org_server, "get_graph_client", get_graph_client) + return acquisitions + + +def _assert_no_private_cache_detail(result, caplog) -> None: + content = result[0] if isinstance(result, tuple) else result.content + for fragment in _PRIVATE_CACHE_DETAIL.split(): + assert fragment not in json.dumps(_structured(result)) + assert all(fragment not in item.text for item in content) + assert fragment not in caplog.text + + +@pytest.mark.parametrize("error_type", [LockException, PermissionError]) +@pytest.mark.parametrize("tool", ["save_bulletin", "duplicate_bulletin", "transition_bulletin"]) +def test_graph_cache_failure_after_commit_preserves_no_repeat_semantics( + fake_clients, monkeypatch, caplog, error_type, tool +) -> None: + client, _ = fake_clients() + arguments = { + "save_bulletin": _save_arguments(), + "duplicate_bulletin": {"id": "bulletin-1"}, + "transition_bulletin": {"id": "bulletin-1", "transition": "archive"}, + }[tool] + + def already_committed(): + assert len(client.saves) + len(client.transitions) == 1 + + acquisitions = _install_failing_graph_acquisition( + monkeypatch, error_type(_PRIVATE_CACHE_DETAIL), + before_failure=already_committed, + ) + result = _call(tool, arguments) + payload = _structured(result) + assert payload["status"] == "failure" + assert payload["tenantId"] == TENANT_ID + assert payload["titleId"] == TITLE_ID + assert payload["errors"][0]["code"] == "CommittedRefreshFailed" + assert payload["errors"][1]["code"] == "AudienceMetadataUnavailable" + assert all(error["retryable"] is False for error in payload["errors"]) + assert "do not repeat" in payload["errors"][0]["message"] + assert len(client.saves) + len(client.transitions) == 1 + assert acquisitions == [TENANT_ID] + _assert_no_private_cache_detail(result, caplog) + + +@pytest.mark.parametrize("error_type", [LockException, PermissionError]) +@pytest.mark.parametrize( + ("tool", "arguments", "expected_code"), + [ + ("search_audience_groups", {"query": "finance"}, "AuthenticationRequired"), + ("open_org_announcements", {"view": "manager"}, "AudienceMetadataUnavailable"), + ("open_org_announcements", { + "view": "editor", "mode": "create", "suggestedDraft": {"audience": ["g1"]} + }, "AudienceMetadataUnavailable"), + ("open_org_announcements", { + "view": "editor", "mode": "edit", "bulletinId": "bulletin-1" + }, "AudienceMetadataUnavailable"), + ], +) +def test_graph_cache_failure_preserves_open_and_search_envelopes( + fake_clients, monkeypatch, caplog, error_type, tool, arguments, expected_code +) -> None: + client, _ = fake_clients() + acquisitions = _install_failing_graph_acquisition( + monkeypatch, error_type(_PRIVATE_CACHE_DETAIL) + ) + result = _call(tool, arguments) + payload = _structured(result) + assert payload["code"] == expected_code + assert payload["retryable"] is False + assert "items" not in payload and "config" not in payload + if tool == "search_audience_groups": + assert set(payload) == {"status", "code", "retryable"} + assert payload["status"] == "failure" + else: + assert payload["view"] == "error" + assert payload["tenantId"] == TENANT_ID + assert payload["request"] == {"titleId": TITLE_ID, **arguments} + assert client.saves == client.transitions == [] + assert acquisitions == [TENANT_ID] + _assert_no_private_cache_detail(result, caplog) + + +@pytest.mark.parametrize("error_type", [LockException, PermissionError]) +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("open_org_announcements", {"view": "editor", "mode": "create"}), + ("save_bulletin", _save_arguments()), + ("duplicate_bulletin", {"id": "bulletin-1"}), + ("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}), + ("search_audience_groups", {"query": "finance"}), + ], +) +def test_authoring_cache_failure_preserves_feature_envelopes( + monkeypatch, caplog, error_type, tool, arguments +) -> None: + attempts = [] + + def construct(): + attempts.append(True) + raise error_type(_PRIVATE_CACHE_DETAIL) + + monkeypatch.setattr(org_server, "_client", None) + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", construct) + result = _call(tool, arguments) + payload = _structured(result) + assert attempts == [True] + assert org_server._client is None + assert not org_server._client_lock.locked() + assert "tenantId" not in payload + assert "manager" not in payload and "item" not in payload + if tool in ("open_org_announcements", "search_audience_groups"): + assert payload["code"] == "AuthenticationRequired" + assert payload["retryable"] is False + else: + assert payload["status"] == "failure" + assert payload["errors"][0]["code"] == "AuthenticationRequired" + assert payload["errors"][0]["retryable"] is False + if tool != "search_audience_groups": + assert payload["titleId"] == TITLE_ID + _assert_no_private_cache_detail(result, caplog) + + +@pytest.mark.parametrize("error_type", [LockException, PermissionError]) +def test_authoring_cache_failure_preserves_its_private_cause(monkeypatch, error_type) -> None: + original = error_type(_PRIVATE_CACHE_DETAIL) + + def construct(): + raise original + + monkeypatch.setattr(org_server, "_client", None) + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", construct) + + async def run(): + with pytest.raises(org_server._FailureResult) as caught: + await org_server.get_client() + assert caught.value.__cause__ is original + assert caught.value.code == "AuthenticationRequired" + assert _PRIVATE_CACHE_DETAIL not in caught.value.message + + asyncio.run(run()) diff --git a/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py b/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py new file mode 100644 index 000000000..3a5553959 --- /dev/null +++ b/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py @@ -0,0 +1,474 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Privacy and fail-open guards for Org Announcements telemetry. + +The tightest constraint on this surface is that announcement content, audience +group identifiers, the tenant's API endpoint, tokens, claims, and the opener +request payload must never leave the machine. These tests assert that at the +emit boundary — the last point where a leak is still catchable — rather than by +reading the calling code, so a future caller that passes the wrong value is +caught here too. +""" + +from __future__ import annotations + +import asyncio +import sys +from contextlib import asynccontextmanager +from datetime import datetime, timezone +from pathlib import Path +from typing import Any + +import pytest + + +REPO_ROOT = Path(__file__).parents[3] +ORG_ANNOUNCEMENTS_DIR = ( + REPO_ROOT + / "solutions" + / "ess-maker-skills" + / "src" + / "mcp" + / "agentconfig_org_announcements" +) +# Sibling MCP servers share the top-level names ``client``/``server``, so the +# modules are loaded through the shared isolated importer rather than by a plain +# ``import`` off ``sys.path``. See tests/mcp/_mcp_modules.py. +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from _mcp_modules import load_org_announcements_modules # noqa: E402 + +_ORG_MODULES = load_org_announcements_modules() +org_telemetry = _ORG_MODULES["telemetry"] +org_server = _ORG_MODULES["server"] +org_client = _ORG_MODULES["client"] + + +NOW = datetime(2026, 9, 4, 12, 0, tzinfo=timezone.utc) +TENANT_ID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" +OBJECT_ID = "00000000-0000-0000-0000-000000003333" +TITLE_ID = "secret-agent-title" + +# Values that must never appear in any emitted field. +SECRET_TITLE = "Layoffs announcement do not leak" +SECRET_DESCRIPTION = "Confidential body text" +SECRET_GROUP_ID = "11111111-2222-3333-4444-555555555555" +SECRET_ENDPOINT = "https://substrate.office.com/weveb2/api/v1.1" +SECRET_TOKEN = "eyJhbGciOiJub25lIn0.payload.signature" # noqa: S105 — fixture +SECRET_PROMPT = "Ask about the reorg" +SECRET_URL = "https://contoso.example/secret-plan" + +FORBIDDEN = ( + TITLE_ID, + TENANT_ID, + OBJECT_ID, + SECRET_TITLE, + SECRET_DESCRIPTION, + SECRET_GROUP_ID, + SECRET_ENDPOINT, + SECRET_TOKEN, + SECRET_PROMPT, + SECRET_URL, +) + + +@pytest.fixture +def emitted(monkeypatch) -> list[dict[str, Any]]: + """Capture every event this module would hand to the ADK emitter.""" + events: list[dict[str, Any]] = [] + + class _FakeAdkTelemetry: + @staticmethod + def emit_api_call(**fields: Any) -> dict[str, Any]: + events.append(dict(fields)) + return {"sent": False} + + monkeypatch.setattr( + org_telemetry, "_adk_telemetry", lambda: _FakeAdkTelemetry() + ) + return events + + +def _flatten(event: dict[str, Any]) -> str: + return " ".join(f"{key}={value}" for key, value in event.items()) + + +# -------------------------------------------------------------------------- +# Field shape +# -------------------------------------------------------------------------- + + +def test_a_success_event_carries_only_operation_outcome_and_latency( + emitted, +) -> None: + org_telemetry.record_operation( + "save_bulletin", outcome="success", latency_ms=42 + ) + + assert emitted == [ + { + "api_endpoint": "save_bulletin", + "outcome": "success", + "latency_ms": 42, + } + ] + + +def test_a_failure_event_adds_only_a_stable_code_and_broad_source( + emitted, +) -> None: + org_telemetry.record_operation( + "save_bulletin", + outcome="failure", + latency_ms=7, + error_code="AudienceRequired", + error_source=org_telemetry.SOURCE_BACKEND, + ) + + assert emitted == [ + { + "api_endpoint": "save_bulletin", + "outcome": "failure", + "latency_ms": 7, + "error_code": "AudienceRequired", + "error_category": "backend", + # Never a message: backend text can echo the announcement back. + "error_message": "", + } + ] + + +def test_the_endpoint_dimension_is_the_tool_name_not_a_url(emitted) -> None: + org_telemetry.record_operation( + "open_org_announcements", outcome="success", latency_ms=1 + ) + + assert emitted[0]["api_endpoint"] == "open_org_announcements" + assert "://" not in emitted[0]["api_endpoint"] + + +@pytest.mark.parametrize( + "operation", + [ + "open_org_announcements", + "save_bulletin", + "transition_bulletin", + "duplicate_bulletin", + "search_audience_groups", + ], +) +def test_every_real_operation_is_reported_verbatim(emitted, operation) -> None: + org_telemetry.record_operation(operation, outcome="success", latency_ms=1) + + assert emitted[0]["api_endpoint"] == operation + + +def test_an_unknown_operation_is_bucketed_rather_than_emitted(emitted) -> None: + """A caller-controlled name must never mint a new dimension value.""" + org_telemetry.record_operation( + SECRET_TITLE, outcome="success", latency_ms=1 + ) + + assert emitted[0]["api_endpoint"] == org_telemetry.OPERATION_UNKNOWN + + +def test_an_unknown_error_source_is_bucketed(emitted) -> None: + org_telemetry.record_operation( + "save_bulletin", + outcome="failure", + latency_ms=1, + error_code="X", + error_source=SECRET_ENDPOINT, + ) + + assert emitted[0]["error_category"] == org_telemetry.SOURCE_MCP + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ("AudienceRequired", "AudienceRequired"), + ("Committed_Refresh_Failed", "Committed_Refresh_Failed"), + ("", ""), + # A message masquerading as a code is replaced, not truncated: a + # truncated message is still content. + ("Announcement 'Layoffs' is invalid", "UnknownError"), + ("https://contoso.example/x", "UnknownError"), + ("a" * 200, "UnknownError"), + ], +) +def test_only_identifier_shaped_error_codes_are_emitted(raw, expected) -> None: + assert org_telemetry.normalize_error_code(raw) == expected + + +def test_a_negative_latency_is_clamped(emitted) -> None: + org_telemetry.record_operation( + "save_bulletin", outcome="success", latency_ms=-5 + ) + + assert emitted[0]["latency_ms"] == 0 + + +# -------------------------------------------------------------------------- +# Fail open +# -------------------------------------------------------------------------- + + +def test_a_missing_adk_telemetry_module_is_not_an_error(monkeypatch) -> None: + monkeypatch.setattr(org_telemetry, "_adk_telemetry", lambda: None) + + org_telemetry.record_operation( + "save_bulletin", outcome="success", latency_ms=1 + ) + + +def test_an_emitter_exception_never_reaches_the_caller(monkeypatch) -> None: + class _Exploding: + @staticmethod + def emit_api_call(**fields: Any) -> None: + raise RuntimeError("collector unreachable") + + monkeypatch.setattr(org_telemetry, "_adk_telemetry", lambda: _Exploding()) + + org_telemetry.record_operation( + "save_bulletin", outcome="failure", latency_ms=1, error_code="X" + ) + + +def test_a_telemetry_failure_does_not_fail_the_tool_call( + monkeypatch, emitted +) -> None: + class _Exploding: + @staticmethod + def emit_api_call(**fields: Any) -> None: + raise RuntimeError("collector unreachable") + + monkeypatch.setattr(org_telemetry, "_adk_telemetry", lambda: _Exploding()) + _install_fakes(monkeypatch) + + payload = _call( + "open_org_announcements", {"view": "manager"} + ) + + assert payload["view"] == "manager" + + +# -------------------------------------------------------------------------- +# End-to-end: nothing sensitive escapes a real tool call +# -------------------------------------------------------------------------- + + +def _config(bulletin_id: str = "bulletin-1", *, status: str = "draft") -> dict: + return { + "titleId": TITLE_ID, + "bulletin": { + "id": bulletin_id, + "type": "standard", + "priority": 1, + "title": SECRET_TITLE, + "description": SECRET_DESCRIPTION, + "startDate": "2026-09-01T00:00:00.000Z", + "primaryAction": { + "actionType": "externalLink", + "label": "Read", + "url": SECRET_URL, + }, + }, + "audience": [SECRET_GROUP_ID], + "status": status, + } + + +class _FakeClient: + tenant_id = TENANT_ID + object_id = OBJECT_ID + + def __init__(self, *, save_error: Exception | None = None) -> None: + self.save_error = save_error + + async def list_bulletins(self, title_id: str) -> list[dict]: + return [_config()] + + async def get_bulletin(self, title_id: str, bulletin_id: str) -> dict: + return _config(bulletin_id) + + async def save_bulletin(self, title_id: str, payload: dict) -> dict: + if self.save_error is not None: + raise self.save_error + return _config(payload.get("id") or "created-1", status=payload["status"]) + + async def transition_bulletin(self, title_id: str, bulletin_id: str, status: str) -> dict: + return _config(bulletin_id, status=status) + + +class _FakeGraphClient: + async def resolve_groups(self, group_ids) -> dict: + return { + SECRET_GROUP_ID: { + "id": SECRET_GROUP_ID, + "displayName": "Leadership", + "mail": None, + "isValid": True, + } + } + + async def search_groups(self, query: str) -> dict: + return {"groups": [], "exhausted": True, "pagesExamined": 1} + + +def _install_fakes(monkeypatch, *, client=None) -> None: + resolved = client or _FakeClient() + + async def _get_client(): + return resolved + + @asynccontextmanager + async def _get_graph_client(tenant_id, object_id): + assert tenant_id == resolved.tenant_id + assert object_id == resolved.object_id + yield _FakeGraphClient() + + monkeypatch.setattr(org_server, "get_client", _get_client) + monkeypatch.setattr(org_server, "get_graph_client", _get_graph_client) + monkeypatch.setattr(org_server, "_now", lambda: NOW) + + +def _call(tool: str, arguments: dict) -> dict: + if tool != "search_audience_groups": + arguments = {"titleId": TITLE_ID, **arguments} + async def run(): + return await org_server.mcp.call_tool(tool, arguments) + + result = asyncio.run(run()) + return result[1] if isinstance(result, tuple) else result.structuredContent + + +def _save_arguments() -> dict: + return { + "bulletin": { + "type": "standard", + "title": SECRET_TITLE, + "description": SECRET_DESCRIPTION, + "primaryAction": { + "actionType": "copilotChat", + "label": "Ask", + "prompt": SECRET_PROMPT, + }, + }, + "audience": [SECRET_GROUP_ID], + "status": "draft", + } + + +def test_a_successful_save_emits_no_content_or_group_identifier( + monkeypatch, emitted +) -> None: + _install_fakes(monkeypatch) + + _call("save_bulletin", _save_arguments()) + + assert emitted, "the save emitted no telemetry at all" + for event in emitted: + flat = _flatten(event) + for secret in FORBIDDEN: + assert secret not in flat, f"{secret!r} leaked into telemetry" + + +def test_a_failed_save_emits_only_the_stable_code(monkeypatch, emitted) -> None: + _install_fakes( + monkeypatch, + client=_FakeClient( + save_error=org_client.BulletinValidationError( + [ + { + "code": "AudienceGroupInvalid", + "field": "audience", + # A backend message can echo content straight back. + "message": f"Group {SECRET_GROUP_ID} is invalid for " + f"{SECRET_TITLE}.", + } + ] + ) + ), + ) + + _call("save_bulletin", _save_arguments()) + + assert emitted + event = emitted[-1] + assert event["error_code"] == "AudienceGroupInvalid" + assert event["error_message"] == "" + for secret in FORBIDDEN: + assert secret not in _flatten(event) + + +def test_the_opener_never_emits_the_suggested_draft(monkeypatch, emitted) -> None: + """The opener request can carry suggested content and audience IDs.""" + _install_fakes(monkeypatch) + + _call( + "open_org_announcements", + { + "view": "editor", + "mode": "create", + "suggestedDraft": { + "title": SECRET_TITLE, + "description": SECRET_DESCRIPTION, + "audience": [SECRET_GROUP_ID], + "primaryAction": { + "actionType": "externalLink", + "label": "Read", + "url": SECRET_URL, + }, + }, + }, + ) + + assert emitted + for event in emitted: + flat = _flatten(event) + for secret in FORBIDDEN: + assert secret not in flat + + +def test_search_telemetry_never_carries_the_query(monkeypatch, emitted) -> None: + _install_fakes(monkeypatch) + + _call("search_audience_groups", {"query": SECRET_TITLE}) + + assert emitted + for event in emitted: + assert SECRET_TITLE not in _flatten(event) + + +def test_a_transition_emits_no_identifier(monkeypatch, emitted) -> None: + _install_fakes(monkeypatch) + + _call("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}) + + assert emitted + for event in emitted: + assert "bulletin-1" not in _flatten(event) + + +def test_no_event_field_is_outside_the_agreed_set(monkeypatch, emitted) -> None: + """A new field is a new disclosure; it must be a deliberate change.""" + _install_fakes(monkeypatch) + + _call("open_org_announcements", {"view": "manager"}) + _call("save_bulletin", _save_arguments()) + _call("transition_bulletin", {"id": "bulletin-1", "transition": "archive"}) + _call("duplicate_bulletin", {"id": "bulletin-1"}) + _call("search_audience_groups", {"query": "finance"}) + + allowed = { + "api_endpoint", + "outcome", + "latency_ms", + "error_code", + "error_category", + "error_message", + } + for event in emitted: + assert set(event) <= allowed, set(event) - allowed diff --git a/tests/mcp/test_import_isolation.py b/tests/mcp/test_import_isolation.py new file mode 100644 index 000000000..a7e591e09 --- /dev/null +++ b/tests/mcp/test_import_isolation.py @@ -0,0 +1,133 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Guards for collision-free imports of the sibling MCP servers. + +The sibling servers under ``src/mcp`` are flat folders that both define +top-level ``client``/``server`` modules. Under a whole-suite run the first one +imported wins ``sys.modules`` and every later suite silently gets the wrong +module — the failure surfaces as an ``AttributeError`` on a name that does +exist, in a different file, which is very hard to read as an import problem. + +These tests pin the isolation itself so it cannot regress into that state. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[2] +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from _mcp_modules import ( # noqa: E402 + load_landing_page_modules, + load_org_announcements_modules, +) + + +def test_both_servers_load_with_their_own_client_module() -> None: + landing = load_landing_page_modules() + org = load_org_announcements_modules() + + assert landing["client"] is not org["client"] + assert hasattr(landing["client"], "AgentConfigClient") + assert hasattr(org["client"], "OrgAnnouncementsClient") + # Each server bound the client that sits next to it, not its sibling's. + assert not hasattr(landing["client"], "OrgAnnouncementsClient") + assert not hasattr(org["client"], "AgentConfigClient") + + +def test_each_server_binds_the_client_from_its_own_directory() -> None: + """``server.py`` does a plain ``from client import ...`` at runtime.""" + landing = load_landing_page_modules() + org = load_org_announcements_modules() + + assert Path(landing["server"].__file__).parent == Path( + landing["client"].__file__ + ).parent + assert Path(org["server"].__file__).parent == Path(org["client"].__file__).parent + assert ( + Path(landing["server"].__file__).parent + != Path(org["server"].__file__).parent + ) + + +def test_loading_is_idempotent_and_returns_the_same_modules() -> None: + """Monkeypatching in one test module must be visible in another.""" + first = load_org_announcements_modules() + second = load_org_announcements_modules() + + for name, module in first.items(): + assert second[name] is module + + +def test_loading_leaves_no_plain_sibling_name_behind() -> None: + """The plain names are shadowed only for the duration of the load.""" + before = { + name: sys.modules.get(name) + for name in ("client", "server", "drafts", "graph_directory_client") + } + + load_landing_page_modules() + load_org_announcements_modules() + + for name, previous in before.items(): + assert sys.modules.get(name) is previous, name + + +def test_the_whole_mcp_tree_collects_in_either_order() -> None: + """Import-order independence, proven by compiling every suite together. + + A plain ``import client`` anywhere in these suites reintroduces the + collision, so this asserts the source shape rather than re-running pytest + (which the harness cannot nest). + """ + suites = sorted((REPO_ROOT / "tests" / "mcp").rglob("test_*.py")) + assert suites, "no MCP test modules found" + + for suite in suites: + source = suite.read_text(encoding="utf-8") + for forbidden in ( + "\nimport client", + "\nimport server", + "\nimport drafts", + "\nimport graph_directory_client", + "\nfrom client import", + "\nfrom server import", + "\nfrom graph_directory_client import", + ): + assert forbidden not in source, ( + f"{suite.relative_to(REPO_ROOT)} imports a top-level MCP module " + f"directly ({forbidden.strip()}); use _mcp_modules instead" + ) + + +def test_the_suites_run_clean_in_a_single_interpreter() -> None: + """Both servers imported into one process must stay distinct. + + Reproduces the real whole-suite condition in a subprocess so a regression + that only appears when both are loaded is caught here. + """ + probe = ( + "import sys; sys.path.insert(0, %r);" + "from _mcp_modules import load_landing_page_modules, " + "load_org_announcements_modules;" + "a = load_landing_page_modules(); b = load_org_announcements_modules();" + "assert a['client'] is not b['client'];" + "assert hasattr(b['client'], 'OrgAnnouncementsClient');" + "assert hasattr(a['client'], 'AgentConfigClient');" + "print('ok')" % str(REPO_ROOT / "tests" / "mcp") + ) + result = subprocess.run( # noqa: S603 — fixed argv, no shell + [sys.executable, "-c", probe], + cwd=REPO_ROOT, + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "ok" From fed6635ef40b0efff38edde118638a8090c57b10 Mon Sep 17 00:00:00 2001 From: Reilly Bova Date: Fri, 18 Sep 2026 10:36:22 -0700 Subject: [PATCH 2/8] fix(announcements): align opener guidance and recovery coverage Describe read-only repairable copies and cover zero-write opening, priority preservation, unresolved audiences, backend action errors, and non-repeating committed-refresh failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15e9d51c-c328-48e6-9948-8819f9e57f90 --- .../agentconfig_org_announcements/server.py | 4 +- .../test_mcp_app_protocol.py | 293 ++++++++++++++++++ 2 files changed, 296 insertions(+), 1 deletion(-) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py index 00957de7d..a003dcaec 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -759,7 +759,9 @@ async def open_org_announcements( calls it at most once per maker turn; the widget uses it for navigation within the same tenant-and-agent scope. Manager takes no editor arguments. Editor requires mode=create without bulletinId, or mode=edit with bulletinId. - suggestedDraft is supported only for create. + suggestedDraft is supported only for create, including editable copies + whose actions need repair. Opening preserves working content; it does not + establish that the content is valid to save or publish. """ # The flat tool signature stays compatible with MCP callers. Validate the # combination before entering the recoverable widget-error path: an invalid diff --git a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py index 5957140be..564d919d7 100644 --- a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py +++ b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py @@ -393,6 +393,34 @@ def test_opener_metadata_exposes_only_normal_create_and_edit_modes(): assert "Manager takes no editor arguments" in opener.description +def test_opener_tools_list_explains_priority_labels(): + tools = asyncio.run(org_server.mcp.list_tools()) + opener = next(tool for tool in tools if tool.name == "open_org_announcements") + priority = opener.inputSchema["$defs"]["SuggestedBulletinDraft"]["properties"]["priority"] + + assert "0 = Important" in priority["description"] + assert "1 = Informational" in priority["description"] + assert "defaults to Informational (1)" in priority["description"] + + +@pytest.mark.parametrize("priority", [0, 1], ids=["important", "informational"]) +def test_pre_hydrated_priority_survives_the_mcp_boundary(fake_clients, priority): + client, _ = fake_clients() + payload = _structured( + _call( + "open_org_announcements", + { + "view": "editor", + "mode": "create", + "suggestedDraft": {"type": "standard", "priority": priority}, + }, + ) + ) + + assert payload["draft"]["standardPriority"] == priority + assert client.saves == [] + + # -------------------------------------------------------------------------- # Opener behavior # -------------------------------------------------------------------------- @@ -449,6 +477,45 @@ def test_empty_create_returns_the_editor_defaults_without_writing( assert client.transitions == [] +def test_empty_audience_search_allows_review_only_create_without_audience(fake_clients) -> None: + client, _ = fake_clients(graph=_FakeGraphClient(search_groups_result=[])) + search = _structured(_call("search_audience_groups", {"query": "No matching group"})) + + assert search["status"] == "success" + assert search["groups"] == [] + assert search["exhausted"] is True + + payload = _structured( + _call( + "open_org_announcements", + { + "view": "editor", + "mode": "create", + "suggestedDraft": { + "type": "standard", + "priority": 1, + "title": "Review with unresolved audience", + "description": "Test announcement only.", + "startDate": "2026-09-17", + "endDate": "2026-09-24", + }, + }, + ) + ) + + assert payload["view"] == "editor" + assert payload["config"] is None + assert payload["draft"]["title"] == "Review with unresolved audience" + assert payload["draft"]["description"] == "Test announcement only." + assert payload["draft"]["standardPriority"] == 1 + assert payload["draft"]["startDate"] == "2026-09-17T00:00:00.000Z" + assert payload["draft"]["endDate"] == "2026-09-24T23:59:59.999Z" + assert payload["draft"]["audience"] == [] + assert payload["draft"].get("id") is None + assert client.saves == [] + assert client.transitions == [] + + def test_pre_hydrated_create_overlays_the_suggestion_and_writes_nothing( fake_clients, ) -> None: @@ -505,6 +572,122 @@ def test_pre_hydrated_create_overlays_the_suggestion_and_writes_nothing( assert client.saves == [] +@pytest.mark.parametrize( + "suggestion", + [ + {}, + {"type": "standard", "title": "", "description": "", "audience": []}, + {"type": "standard", "startDate": "", "endDate": "", "primaryAction": None}, + ], +) +def test_incomplete_working_copy_opens_without_publication_requirements( + fake_clients, suggestion +) -> None: + client, graph = fake_clients() + + payload = _structured(_call("open_org_announcements", { + "view": "editor", "mode": "create", "suggestedDraft": suggestion, + })) + + assert payload["mode"] == "create" + assert payload["view"] == "editor" + assert payload["config"] is None + assert "id" not in payload["draft"] + assert payload["draft"]["title"] == payload["draft"]["description"] == "" + assert payload["draft"]["startDate"] == payload["draft"]["endDate"] == "" + assert payload["draft"]["audience"] == [] + assert payload["draft"]["primaryAction"] is None + assert (payload["tenantId"], payload["titleId"]) == (TENANT_ID, TITLE_ID) + assert graph.tenant_ids == [TENANT_ID] + assert graph.object_ids == [OBJECT_ID] + assert graph.resolve_calls == [] + assert client.gets == client.saves == client.transitions == [] + assert client.lists == 0 + + +@pytest.mark.parametrize( + ("announcement_type", "action"), + [ + ("alert", {"actionType": "copilotChat", "label": "Ask Copilot", "prompt": "Explain the announcement."}), + ("alert", {"actionType": "copilotChat", "label": "Ask Copilot"}), + ("alert", {"actionType": "externalLink", "label": "Open", "url": None}), + ("standard", {"actionType": "externalLink", "label": "Open"}), + ("standard", {"actionType": "copilotChat", "label": "", "prompt": ""}), + ], +) +def test_frontend_copy_projection_opens_repairable_actions_with_audience_hydration( + fake_clients, announcement_type, action +) -> None: + """Vorpal openRowEditor projects copy content, not a persisted source ID.""" + client, graph = fake_clients() + suggestion = { + "type": announcement_type, + "title": "Kopie der Ankuendigung", + "description": "Editable working content", + "primaryAction": action, + "startDate": "2026-09-10T00:00:00.000Z", + "endDate": "2026-10-10T23:59:59.999Z", + "audience": ["g1"], + } + original = copy.deepcopy(suggestion) + + payload = _structured(_call("open_org_announcements", { + "view": "editor", "mode": "create", "suggestedDraft": suggestion, + })) + + assert (payload["view"], payload["mode"], payload["config"]) == ("editor", "create", None) + assert (payload["tenantId"], payload["titleId"]) == (TENANT_ID, TITLE_ID) + draft = payload["draft"] + assert "id" not in draft + for field in ("type", "title", "description", "startDate", "endDate"): + assert draft[field] == suggestion[field] + assert draft["primaryAction"] == {"url": None, "prompt": None, **action} + assert draft["standardPriority"] == 1 + assert draft["standardSecondaryAction"] is None + assert draft["audience"] == [graph.resolved["g1"]] + assert graph.resolve_calls == [["g1"]] + assert graph.tenant_ids == [TENANT_ID] + assert graph.object_ids == [OBJECT_ID] + assert client.gets == client.saves == client.transitions == [] + assert client.lists == 0 + assert suggestion == original + + +def test_repairable_copy_directory_failure_is_an_honest_read_only_open_error(fake_clients) -> None: + client, graph = fake_clients(graph=_FakeGraphClient( + error=GraphDirectoryError("Synthetic directory outage", code="SearchUnavailable", retryable=True) + )) + original = { + "type": "alert", + "title": "Copy of Repairable announcement", + "primaryAction": {"actionType": "copilotChat", "label": "Ask Copilot"}, + "audience": ["g1"], + } + + result = asyncio.run(org_server.open_org_announcements( + titleId=TITLE_ID, + view="editor", + mode="create", + suggestedDraft=org_server.SuggestedBulletinDraft.model_validate(original), + )) + + assert result.isError is True + payload = result.structuredContent + assert set(payload) == {"tenantId", "titleId", "view", "request", "code", "message", "retryable"} + assert payload["view"] == "error" + assert payload["code"] == "AudienceMetadataUnavailable" + assert payload["retryable"] is True + assert payload["request"] == { + "titleId": TITLE_ID, "view": "editor", "mode": "create", "suggestedDraft": original, + } + assert (payload["tenantId"], payload["titleId"]) == (TENANT_ID, TITLE_ID) + assert "Synthetic directory outage" in payload["message"] + assert result.content[0].text == payload["message"] + assert graph.resolve_calls == [["g1"]] + assert client.gets == client.saves == client.transitions == [] + assert client.lists == 0 + + def test_a_suggested_draft_carrying_canonical_metadata_is_rejected( fake_clients, ) -> None: @@ -1031,6 +1214,63 @@ def test_http_200_validation_errors_are_reported_individually(fake_clients) -> N assert all(error["retryable"] is False for error in payload["errors"]) +@pytest.mark.parametrize("status", ["draft", "published"]) +@pytest.mark.parametrize( + ("announcement_type", "action", "code"), + [ + ("alert", {"actionType": "copilotChat", "label": "Ask", "prompt": "Explain"}, "AlertActionInvalid"), + ("standard", {"actionType": "externalLink", "label": "Open"}, "ActionTargetMissing"), + ("standard", {"actionType": "externalLink", "label": "Open", "url": "not-a-url"}, "ActionUrlNotHttps"), + ], +) +def test_opening_repairable_content_does_not_bypass_backend_action_validation( + fake_clients, status, announcement_type, action, code +) -> None: + """WeveNova b6fc27a: EmployeeAgentBulletinAccessor.Validation.cs, + ValidateBulletin -> ValidateActions validates present actions for both statuses. + The error codes/field are source-derived; the message and response are synthetic. + """ + backend_error = { + "code": code, + "field": "primaryAction", + "message": "Synthetic action validation failure.", + } + client, graph = fake_clients(_FakeClient( + save_error=org_client.BulletinValidationError([backend_error]) + )) + bulletin = { + "type": announcement_type, + "title": "Copy for repair", + "description": "Editable working content", + "primaryAction": action, + } + opened = _structured(_call("open_org_announcements", { + "view": "editor", "mode": "create", "suggestedDraft": bulletin, + })) + assert opened["view"] == "editor" + assert opened["config"] is None + assert "id" not in opened["draft"] + assert client.saves == [] + + result = asyncio.run(org_server.save_bulletin( + titleId=TITLE_ID, bulletin=bulletin, audience=[], status=status, + )) + + assert result.isError is True + assert result.structuredContent == { + "tenantId": TENANT_ID, + "titleId": TITLE_ID, + "status": "failure", + "errors": [{**backend_error, "retryable": False}], + } + assert len(client.saves) == 1 + assert client.saves[0]["bulletin"] == bulletin + assert client.saves[0]["status"] == status + assert client.gets == client.transitions == [] + assert client.lists == 0 + assert graph.resolve_calls == [] + + def test_a_server_failure_is_not_reported_as_a_validation_error( fake_clients, ) -> None: @@ -1152,6 +1392,59 @@ def test_a_graph_failure_after_a_committed_save_is_not_retryable( assert len(client.saves) == 1 +@pytest.mark.parametrize("saved_audience", [[], ["g1"]], ids=["manager-hydration", "saved-item-hydration"]) +@pytest.mark.parametrize("identifier", [None, "existing-1"], ids=["create", "update"]) +def test_committed_save_graph_failure_preserves_full_envelope_and_never_replays( + fake_clients, saved_audience, identifier +) -> None: + class EchoSavedClient(_FakeClient): + async def save_bulletin(self, title_id, payload): + saved = await super().save_bulletin(title_id, payload) + saved["audience"] = list(payload["audience"]) + return saved + + client, graph = fake_clients( + EchoSavedClient(items=[_config("another-announcement", audience=["g1"])]), + graph=_FakeGraphClient( + error=GraphDirectoryError("Synthetic directory outage", code="SearchUnavailable", retryable=True) + ), + ) + arguments = _save_arguments(audience=saved_audience) + if identifier is not None: + arguments["id"] = identifier + + result = asyncio.run(org_server.save_bulletin(titleId=TITLE_ID, **arguments)) + + assert result.isError is True + payload = result.structuredContent + assert set(payload) == {"tenantId", "titleId", "status", "errors"} + assert (payload["tenantId"], payload["titleId"]) == (TENANT_ID, TITLE_ID) + assert payload["status"] == "failure" + summary, cause = payload["errors"] + assert summary == { + "code": "CommittedRefreshFailed", + "field": None, + "message": result.content[0].text, + "retryable": False, + } + assert "saved" in summary["message"] + assert "do not repeat the action" in summary["message"] + assert cause == { + "code": "AudienceMetadataUnavailable", + "field": None, + "message": "Audience group names could not be loaded. Synthetic directory outage", + "retryable": False, + } + assert len(client.saves) == 1 + assert client.saves[0].get("id") == identifier + assert client.saves[0]["audience"] == saved_audience + assert client.lists == (0 if saved_audience else 1) + assert graph.resolve_calls == [["g1"]] + assert graph.tenant_ids == [TENANT_ID] + assert graph.object_ids == [OBJECT_ID] + assert client.gets == client.transitions == [] + + def test_a_refresh_failure_after_a_committed_duplicate_is_not_retryable( fake_clients, ) -> None: From 78d5c317ce24d5728f702dde4e824c90b8ec5651 Mon Sep 17 00:00:00 2001 From: Sophie Song Date: Fri, 25 Sep 2026 05:35:50 -0700 Subject: [PATCH 3/8] Harden announcement runtime error handling Sanitize model-visible backend failures and hide validation input values while preserving structured app-only mutation errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19335911-b012-4b7f-b179-e4caeae7ea69 --- .../agentconfig_org_announcements/drafts.py | 2 +- .../agentconfig_org_announcements/server.py | 41 +++++++++- .../test_drafts.py | 18 +++++ .../test_mcp_app_protocol.py | 76 +++++++++++++++++++ 4 files changed, 132 insertions(+), 5 deletions(-) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/drafts.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/drafts.py index faaeb4077..81c67a830 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/drafts.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/drafts.py @@ -136,7 +136,7 @@ def normalize_suggested_instant(value: str, *, boundary: str) -> str: class StrictModel(BaseModel): """Reject any field the contract does not name.""" - model_config = ConfigDict(extra="forbid") + model_config = ConfigDict(extra="forbid", hide_input_in_errors=True) class BulletinAction(StrictModel): diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py index a003dcaec..efa4bfa94 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -141,6 +141,22 @@ # reported to the maker as if they had typed something wrong. _FALLBACK_BACKEND_CODE = "HttpError" +# Backend failure copy safe for the model-visible opener. The opener rebuilds +# even recognized failures from this local map rather than trusting the message +# paired with a familiar backend code. Every other backend code/detail remains +# available only to app-visible mutation tools. +_MODEL_VISIBLE_BACKEND_MESSAGES = { + "AuthenticationRequired": "Sign in again to author organization announcements.", + "AuthorizationDenied": ( + "This account is not authorized to author organization " + "announcements in this tenant." + ), + "FeatureUnavailable": "Organization announcements are not enabled for this tenant.", + "NetworkError": "The Org Announcements service is temporarily unavailable.", + "NotFound": "The announcement was not found.", + "ServiceError": "The Org Announcements service could not complete the request.", +} + # Identity and audit fields the backend owns. A duplicate strips them from the # copied content so the copy is created as a fresh Draft rather than silently # updating its source or inheriting its history. The wrapper-level audit fields @@ -669,16 +685,33 @@ def _open_error_payload( The request is response-only retry state, including the original suggestion. It is never logged or sent to telemetry. """ + visible_failure = failure + if failure.source == SOURCE_BACKEND: + safe_message = _MODEL_VISIBLE_BACKEND_MESSAGES.get(failure.code) + if safe_message is None: + visible_failure = _FailureResult( + "InvalidRequest", + "The Org Announcements service rejected the request.", + source=SOURCE_BACKEND, + ) + else: + visible_failure = _FailureResult( + failure.code, + safe_message, + retryable=failure.retryable, + source=SOURCE_BACKEND, + ) + payload = { **scope, "view": "error", "request": request, - "code": failure.code, - "message": failure.message, - "retryable": failure.retryable, + "code": visible_failure.code, + "message": visible_failure.message, + "retryable": visible_failure.retryable, } return CallToolResult( - content=[TextContent(type="text", text=failure.message)], + content=[TextContent(type="text", text=visible_failure.message)], structuredContent=payload, isError=True, ) diff --git a/tests/mcp/agentconfig_org_announcements/test_drafts.py b/tests/mcp/agentconfig_org_announcements/test_drafts.py index dd1a24393..2ccb80ef2 100644 --- a/tests/mcp/agentconfig_org_announcements/test_drafts.py +++ b/tests/mcp/agentconfig_org_announcements/test_drafts.py @@ -91,6 +91,24 @@ def test_a_suggested_draft_accepts_only_the_agreed_contract_fields() -> None: } +@pytest.mark.parametrize( + ("payload", "sensitive_value"), + [ + ({"type": "sensitive-invalid-type"}, "sensitive-invalid-type"), + ({"title": {"sensitive": "wrong type"}}, "wrong type"), + ( + {"title": "Hi", "internalNote": "sensitive extra field"}, + "sensitive extra field", + ), + ], +) +def test_validation_errors_do_not_echo_input_values(payload, sensitive_value) -> None: + with pytest.raises(ValidationError) as caught: + drafts.SuggestedBulletinDraft.model_validate(payload) + + assert sensitive_value not in str(caught.value) + + # -------------------------------------------------------------------------- # Defaults and field mapping # -------------------------------------------------------------------------- diff --git a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py index 564d919d7..f6df87c60 100644 --- a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py +++ b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py @@ -918,6 +918,58 @@ def test_backend_failures_map_to_discriminated_open_errors( assert payload["code"] == expected_code +@pytest.mark.parametrize("http_status", [400, 409, 422]) +def test_open_errors_do_not_expose_backend_details(fake_clients, http_status) -> None: + sensitive_detail = "sensitive backend diagnostic with request content" + fake_clients( + _FakeClient( + get_error=org_client.AgentConfigApiError( + f"PrivateBackendCode: {sensitive_detail}", + http_status=http_status, + ) + ) + ) + + result = _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "b"}, + ) + payload = _structured(result) + + assert payload["code"] == "InvalidRequest" + assert payload["message"] == "The Org Announcements service rejected the request." + assert sensitive_detail not in json.dumps(payload) + content = result[0] if isinstance(result, tuple) else result.content + assert sensitive_detail not in content[0].text + + +def test_open_errors_rebuild_messages_for_recognized_backend_codes( + fake_clients, +) -> None: + sensitive_detail = "sensitive detail paired with a familiar code" + fake_clients( + _FakeClient( + get_error=org_client.AgentConfigApiError( + f"ServiceError: {sensitive_detail}", + http_status=400, + ) + ) + ) + + payload = _structured( + _call( + "open_org_announcements", + {"view": "editor", "mode": "edit", "bulletinId": "b"}, + ) + ) + + assert payload["code"] == "ServiceError" + assert payload["message"] == ( + "The Org Announcements service could not complete the request." + ) + assert sensitive_detail not in json.dumps(payload) + + @pytest.mark.parametrize("backend_code", ["FeatureDisabled", "FeatureNotEnabled"]) def test_a_feature_gated_tenant_reports_feature_unavailable( fake_clients, backend_code @@ -1068,6 +1120,30 @@ def test_a_draft_with_blank_dates_sends_no_empty_string_to_the_api( assert '""' not in json.dumps(sent) +def test_save_validation_errors_do_not_echo_invalid_input(fake_clients) -> None: + sensitive_value = "sensitive draft value that must not be echoed" + client, _ = fake_clients() + + payload = _structured( + _call( + "save_bulletin", + _save_arguments( + bulletin={ + "type": "standard", + "title": "Quarterly update", + "description": "Read this", + "internalNote": sensitive_value, + } + ), + ) + ) + + assert payload["status"] == "failure" + assert payload["errors"][0]["code"] == "InvalidRequest" + assert sensitive_value not in json.dumps(payload) + assert client.saves == [] + + def test_a_published_save_with_blank_dates_still_reaches_the_backend( fake_clients, ) -> None: From 4e9e51c48e1c91ef8450077ef61935206e30c58e Mon Sep 17 00:00:00 2001 From: Sophie Song Date: Fri, 25 Sep 2026 11:31:30 -0700 Subject: [PATCH 4/8] fix(announcements): harden runtime trust boundaries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19335911-b012-4b7f-b179-e4caeae7ea69 --- .../agentconfig_org_announcements/client.py | 35 ++-------- .../agentconfig_org_announcements/server.py | 50 ++++++++++++--- .../validation.py | 49 ++++++++++++++ tests/mcp/_mcp_modules.py | 5 +- .../test_authoring_client.py | 16 ++++- .../test_mcp_app_protocol.py | 64 ++++++++++++++++++- 6 files changed, 173 insertions(+), 46 deletions(-) create mode 100644 solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/client.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/client.py index d7e18642c..d5be7237d 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/client.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/client.py @@ -37,10 +37,10 @@ from _odata import ( # noqa: E402 _require_odata_id, _validate_https_base_url, - _validate_title_id, ) from agent_discovery import AgentDiscoveryClient # noqa: E402 from base_client import AgentConfigApiError # noqa: E402 +from validation import validate_bulletin_id, validate_title_id # noqa: E402 DEFAULT_ORG_ANNOUNCEMENTS_BASE_URL = "https://substrate.office.com/weveb2/api/v1.1" @@ -50,9 +50,6 @@ # truncated; it does not reveal an exact archived total. ARCHIVED_WINDOW_SIZE = 50 -_MAX_BULLETIN_ID_LENGTH = 256 - - class IndeterminateWriteError(AgentConfigApiError): """An unkeyed create may have committed but was never acknowledged. @@ -83,30 +80,6 @@ def __init__(self, errors: list[dict[str, Any]]): self.errors = errors -def _validate_bulletin_id(bulletin_id: str) -> str: - """Validate a path-bound bulletin ID. - - The ID is a backend-assigned opaque identifier, so this rejects anything - that could reshape the route (empty, padded, control characters, or a path - separator) rather than trying to canonicalize it. - """ - if not isinstance(bulletin_id, str) or not bulletin_id: - raise ValueError("bulletinId must be a non-empty string") - if bulletin_id != bulletin_id.strip(): - raise ValueError("bulletinId must not have surrounding whitespace") - if len(bulletin_id) > _MAX_BULLETIN_ID_LENGTH: - raise ValueError( - f"bulletinId must not exceed {_MAX_BULLETIN_ID_LENGTH} characters" - ) - if any( - ord(character) < 0x20 or ord(character) == 0x7F for character in bulletin_id - ): - raise ValueError("bulletinId must not contain control characters") - if "/" in bulletin_id or "\\" in bulletin_id or "?" in bulletin_id: - raise ValueError("bulletinId must not contain path or query separators") - return bulletin_id - - def _parse_instant(value: Any) -> Optional[datetime]: """Parse a UTC ISO instant, returning ``None`` for absent or unparseable text. @@ -182,7 +155,7 @@ def __init__(self, *, transport: Optional[httpx.AsyncBaseTransport] = None): ) def _collection_path(self, title_id: str) -> str: - encoded = _require_odata_id(_validate_title_id(title_id), "titleId") + encoded = _require_odata_id(validate_title_id(title_id), "titleId") return f"tenants('{self.tenant_id}')/EmployeeAgents('{encoded}')/essbulletins" @staticmethod @@ -343,7 +316,7 @@ async def list_bulletins(self, title_id: str) -> list[dict[str, Any]]: async def get_bulletin(self, title_id: str, bulletin_id: str) -> dict[str, Any]: """Load one canonical stored configuration.""" - path = f"{self._collection_path(title_id)}/{_validate_bulletin_id(bulletin_id)}" + path = f"{self._collection_path(title_id)}/{validate_bulletin_id(bulletin_id)}" return self._require_config( await self._request("GET", path, transform_payload=False), title_id ) @@ -397,7 +370,7 @@ async def transition_bulletin( save returns, so it is unwrapped identically — including the ID check that proves the transition landed on the requested record. """ - validated_id = _validate_bulletin_id(bulletin_id) + validated_id = validate_bulletin_id(bulletin_id) return self._unwrap_save_result( await self._request( "POST", diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py index efa4bfa94..4f16fdc87 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -49,8 +49,6 @@ OrgAnnouncementsClient, build_manager_state, is_deleted_item, - _validate_title_id, - _validate_bulletin_id, ) from drafts import ( AnnouncementEditorDraft, @@ -75,9 +73,18 @@ SOURCE_MCP, record_operation, ) +from validation import validate_bulletin_id, validate_title_id DEFAULT_WIDGET_ORIGIN = "https://workforceinsights.m365.cloud.microsoft" +ALLOWED_WIDGET_ORIGINS = frozenset( + { + "https://workforceinsights.m365.cloud.dev.microsoft", + "https://df.workforceinsights.m365.cloud.microsoft", + DEFAULT_WIDGET_ORIGIN, + } +) +DEVELOPMENT_WIDGET_ORIGIN_ENV = "VORPAL_WIDGET_ALLOW_DEVELOPMENT_ORIGIN" WIDGET_MIME_TYPE = "text/html;profile=mcp-app" ORG_ANNOUNCEMENTS_RESOURCE_URI = ( "ui://widget/org-announcements/OrgAnnouncements.html" @@ -167,7 +174,11 @@ ) -def _resolve_widget_origin(value: Optional[str] = None) -> str: +def _resolve_widget_origin( + value: Optional[str] = None, + *, + allow_development: Optional[bool] = None, +) -> str: origin = ( value or os.environ.get("VORPAL_WIDGET_ORIGIN") or DEFAULT_WIDGET_ORIGIN ).rstrip("/") @@ -185,7 +196,28 @@ def _resolve_widget_origin(value: Optional[str] = None) -> str: "VORPAL_WIDGET_ORIGIN must be an HTTPS origin without credentials, " "a path, a query, or a fragment." ) - return origin + + normalized_origin = f"https://{parsed.netloc.lower()}" + if normalized_origin in ALLOWED_WIDGET_ORIGINS: + return normalized_origin + + if allow_development is None: + allow_development = ( + os.environ.get(DEVELOPMENT_WIDGET_ORIGIN_ENV, "").strip() == "1" + ) + hostname = parsed.hostname.lower() + is_local_development_origin = ( + hostname in {"localhost", "127.0.0.1", "::1"} + or hostname.endswith(".devtunnels.ms") + ) + if allow_development and is_local_development_origin: + return normalized_origin + + raise ValueError( + "VORPAL_WIDGET_ORIGIN must use an approved Vorpal deployment origin. " + f"Set {DEVELOPMENT_WIDGET_ORIGIN_ENV}=1 for localhost, loopback, or " + "*.devtunnels.ms development origins." + ) WIDGET_ORIGIN = _resolve_widget_origin() @@ -800,9 +832,9 @@ async def open_org_announcements( # combination before entering the recoverable widget-error path: an invalid # request is not safe retry state and cannot be rendered as an error view. try: - _validate_title_id(titleId) + validate_title_id(titleId) if bulletinId is not None: - _validate_bulletin_id(bulletinId) + validate_bulletin_id(bulletinId) validated = OpenAnnouncementsRequest( titleId=titleId, view=view, mode=mode, bulletinId=bulletinId, suggestedDraft=suggestedDraft, @@ -1020,7 +1052,7 @@ async def save_bulletin( started = time.monotonic() scope = {"titleId": titleId} try: - _validate_title_id(titleId) + validate_title_id(titleId) request = SaveBulletinRequest.model_validate( { "id": id, @@ -1110,7 +1142,7 @@ async def transition_bulletin( started = time.monotonic() scope = {"titleId": titleId} try: - _validate_title_id(titleId) + validate_title_id(titleId) client = await get_client() scope = {"tenantId": client.tenant_id, "titleId": titleId} changed = await client.transition_bulletin( @@ -1177,7 +1209,7 @@ async def duplicate_bulletin( started = time.monotonic() scope = {"titleId": titleId} try: - _validate_title_id(titleId) + validate_title_id(titleId) client = await get_client() scope = {"tenantId": client.tenant_id, "titleId": titleId} source = await client.get_bulletin(titleId, id) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py new file mode 100644 index 000000000..8adf95e98 --- /dev/null +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py @@ -0,0 +1,49 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +"""Request-boundary validation shared by the announcements client and server.""" + +from __future__ import annotations + +import os +import sys + +# The AgentConfiguration MCP family lives at the ``src/mcp`` root as sibling +# folders sharing the neutral ``agentconfig_core`` client core. There is no +# package __init__.py, and each server launches with cwd set to its own folder +# on a flat sys.path, so make the sibling ``agentconfig_core`` folder importable. +sys.path.insert( + 0, + os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "agentconfig_core"), +) + +from _odata import _validate_title_id # noqa: E402 + + +_MAX_BULLETIN_ID_LENGTH = 256 + + +def validate_title_id(title_id: str) -> str: + """Validate the opaque Employee Agent route key.""" + return _validate_title_id(title_id) + + +def validate_bulletin_id(bulletin_id: str) -> str: + """Validate a path-bound backend-assigned bulletin identifier.""" + if not isinstance(bulletin_id, str) or not bulletin_id: + raise ValueError("bulletinId must be a non-empty string") + if bulletin_id != bulletin_id.strip(): + raise ValueError("bulletinId must not have surrounding whitespace") + if len(bulletin_id) > _MAX_BULLETIN_ID_LENGTH: + raise ValueError( + f"bulletinId must not exceed {_MAX_BULLETIN_ID_LENGTH} characters" + ) + if any( + ord(character) < 0x20 or ord(character) == 0x7F for character in bulletin_id + ): + raise ValueError("bulletinId must not contain control characters") + if any(separator in bulletin_id for separator in ("/", "\\", "?", "#", "%")): + raise ValueError( + "bulletinId must not contain path, query, fragment, or escape separators" + ) + return bulletin_id diff --git a/tests/mcp/_mcp_modules.py b/tests/mcp/_mcp_modules.py index 4ee74dd71..086f07d57 100644 --- a/tests/mcp/_mcp_modules.py +++ b/tests/mcp/_mcp_modules.py @@ -48,6 +48,7 @@ # so its siblings are already registered under their plain names when it runs. LANDING_PAGE_MODULES = ("client", "drafts", "server") ORG_ANNOUNCEMENTS_MODULES = ( + "validation", "client", "drafts", "graph_directory_client", @@ -132,7 +133,7 @@ def load_org_announcements_client_modules() -> dict[str, ModuleType]: """Load contract/client tests without importing optional runtime modules.""" return load_mcp_modules( MCP_ROOT / "agentconfig_org_announcements", - ("client", "drafts"), + ("validation", "client", "drafts"), "ess_mcp_org_announcements", ) @@ -141,7 +142,7 @@ def load_org_announcements_directory_modules() -> dict[str, ModuleType]: """Load directory tests without depending on MCP tools or telemetry.""" return load_mcp_modules( MCP_ROOT / "agentconfig_org_announcements", - ("client", "drafts", "graph_directory_client"), + ("validation", "client", "drafts", "graph_directory_client"), "ess_mcp_org_announcements", ) diff --git a/tests/mcp/agentconfig_org_announcements/test_authoring_client.py b/tests/mcp/agentconfig_org_announcements/test_authoring_client.py index 70efaa6ae..637891bb4 100644 --- a/tests/mcp/agentconfig_org_announcements/test_authoring_client.py +++ b/tests/mcp/agentconfig_org_announcements/test_authoring_client.py @@ -35,6 +35,7 @@ _ORG_MODULES = load_org_announcements_client_modules() org_client = _ORG_MODULES["client"] +org_validation = _ORG_MODULES["validation"] TENANT_ID = "11111111-2222-3333-4444-555555555555" @@ -45,6 +46,16 @@ _UNSET = object() +def test_request_validators_are_shared_public_boundary_helpers() -> None: + assert org_validation.validate_title_id(TITLE_ID) == TITLE_ID + assert org_validation.validate_bulletin_id("bulletin-1") == "bulletin-1" + + with pytest.raises(ValueError, match="titleId"): + org_validation.validate_title_id(" padded") + with pytest.raises(ValueError, match="bulletinId"): + org_validation.validate_bulletin_id("bad/id") + + def _token(tenant_id: str = TENANT_ID) -> str: payload = base64.urlsafe_b64encode( json.dumps({"tid": tenant_id}).encode("utf-8") @@ -158,7 +169,10 @@ async def run() -> None: assert captured[0].url.query == b"" -@pytest.mark.parametrize("bad_id", ["", " a", "a/b", "a\\b", "a?b", "a\x01b"]) +@pytest.mark.parametrize( + "bad_id", + ["", " a", "a/b", "a\\b", "a?b", "a#b", "a%2Fb", "a\x01b"], +) def test_rejects_ids_that_could_reshape_the_route(monkeypatch, bad_id) -> None: client = _make_client( monkeypatch, lambda request: httpx.Response(200, json=_config("a")) diff --git a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py index f6df87c60..e587d1a08 100644 --- a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py +++ b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py @@ -304,11 +304,69 @@ def test_the_resource_shell_loads_the_hosted_bundle_from_the_origin() -> None: ) -def test_widget_origin_override_is_validated() -> None: +@pytest.mark.parametrize( + "origin", + [ + "https://workforceinsights.m365.cloud.dev.microsoft", + "https://df.workforceinsights.m365.cloud.microsoft", + "https://workforceinsights.m365.cloud.microsoft", + ], +) +def test_widget_origin_accepts_known_vorpal_deployment_rings(origin) -> None: + assert org_server._resolve_widget_origin(origin) == origin + + +@pytest.mark.parametrize( + "origin", + [ + "https://example.com", + "https://workforceinsights.m365.cloud.microsoft.evil.example", + "https://vorpal.devtunnels.ms.evil.example", + ], +) +def test_widget_origin_rejects_unapproved_https_hosts(origin) -> None: + for allow_development in [False, True]: + with pytest.raises(ValueError, match="approved Vorpal deployment origin"): + org_server._resolve_widget_origin( + origin, + allow_development=allow_development, + ) + + +@pytest.mark.parametrize( + "origin", + [ + "https://localhost:4200", + "https://127.0.0.1:4200", + "https://[::1]:4200", + "https://vorpal-sophie-5173.euw.devtunnels.ms", + ], +) +def test_widget_origin_allows_local_hosts_only_under_the_development_gate( + origin, +) -> None: + with pytest.raises(ValueError, match="approved Vorpal deployment origin"): + org_server._resolve_widget_origin(origin, allow_development=False) + + assert ( + org_server._resolve_widget_origin(origin, allow_development=True) == origin + ) + + +def test_widget_origin_development_gate_can_be_enabled_by_environment( + monkeypatch, +) -> None: + monkeypatch.setenv(org_server.DEVELOPMENT_WIDGET_ORIGIN_ENV, "1") + assert ( - org_server._resolve_widget_origin("https://localhost:4200") - == "https://localhost:4200" + org_server._resolve_widget_origin( + "https://vorpal-sophie-5173.euw.devtunnels.ms" + ) + == "https://vorpal-sophie-5173.euw.devtunnels.ms" ) + + +def test_widget_origin_shape_is_validated() -> None: for bad in [ "http://localhost:4200", "https://user:pw@example.com", From 4c35c32713df2a3ac768d41525411507a349ea28 Mon Sep 17 00:00:00 2001 From: Sophie Song Date: Fri, 25 Sep 2026 11:55:00 -0700 Subject: [PATCH 5/8] fix(announcements): validate mutation route keys Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19335911-b012-4b7f-b179-e4caeae7ea69 --- .../agentconfig_org_announcements/server.py | 4 ++++ .../validation.py | 2 ++ .../test_authoring_client.py | 2 +- .../test_mcp_app_protocol.py | 24 +++++++++++++++++++ 4 files changed, 31 insertions(+), 1 deletion(-) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py index 4f16fdc87..5dba81174 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -1053,6 +1053,8 @@ async def save_bulletin( scope = {"titleId": titleId} try: validate_title_id(titleId) + if id is not None: + validate_bulletin_id(id) request = SaveBulletinRequest.model_validate( { "id": id, @@ -1143,6 +1145,7 @@ async def transition_bulletin( scope = {"titleId": titleId} try: validate_title_id(titleId) + validate_bulletin_id(id) client = await get_client() scope = {"tenantId": client.tenant_id, "titleId": titleId} changed = await client.transition_bulletin( @@ -1210,6 +1213,7 @@ async def duplicate_bulletin( scope = {"titleId": titleId} try: validate_title_id(titleId) + validate_bulletin_id(id) client = await get_client() scope = {"tenantId": client.tenant_id, "titleId": titleId} source = await client.get_bulletin(titleId, id) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py index 8adf95e98..94cf986e2 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/validation.py @@ -34,6 +34,8 @@ def validate_bulletin_id(bulletin_id: str) -> str: raise ValueError("bulletinId must be a non-empty string") if bulletin_id != bulletin_id.strip(): raise ValueError("bulletinId must not have surrounding whitespace") + if bulletin_id in {".", ".."}: + raise ValueError("bulletinId must not be a URL dot-segment") if len(bulletin_id) > _MAX_BULLETIN_ID_LENGTH: raise ValueError( f"bulletinId must not exceed {_MAX_BULLETIN_ID_LENGTH} characters" diff --git a/tests/mcp/agentconfig_org_announcements/test_authoring_client.py b/tests/mcp/agentconfig_org_announcements/test_authoring_client.py index 637891bb4..d2a5c395d 100644 --- a/tests/mcp/agentconfig_org_announcements/test_authoring_client.py +++ b/tests/mcp/agentconfig_org_announcements/test_authoring_client.py @@ -171,7 +171,7 @@ async def run() -> None: @pytest.mark.parametrize( "bad_id", - ["", " a", "a/b", "a\\b", "a?b", "a#b", "a%2Fb", "a\x01b"], + ["", " a", ".", "..", "a/b", "a\\b", "a?b", "a#b", "a%2Fb", "a\x01b"], ) def test_rejects_ids_that_could_reshape_the_route(monkeypatch, bad_id) -> None: client = _make_client( diff --git a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py index e587d1a08..82f76cb0f 100644 --- a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py +++ b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py @@ -2058,6 +2058,30 @@ async def forbidden_client(): assert payload.get("code") == "InvalidRequest" or payload["errors"][0]["code"] == "InvalidRequest" +@pytest.mark.parametrize("bulletin_id", [".", "..", "a/b", "a%2Fb"]) +@pytest.mark.parametrize("tool", ["save_bulletin", "transition_bulletin", "duplicate_bulletin"]) +def test_invalid_mutation_id_never_acquires_a_client( + monkeypatch, bulletin_id, tool +) -> None: + async def forbidden_client(): + pytest.fail("invalid bulletin ID attempted authentication") + + arguments = { + "save_bulletin": _save_arguments(id=bulletin_id), + "transition_bulletin": {"id": bulletin_id, "transition": "archive"}, + "duplicate_bulletin": {"id": bulletin_id}, + } + monkeypatch.setattr(org_server, "get_client", forbidden_client) + + payload = _structured(_call(tool, arguments[tool])) + + assert payload["errors"][0]["code"] == "InvalidRequest" + assert payload["titleId"] == TITLE_ID + assert "tenantId" not in payload + assert "item" not in payload + assert "manager" not in payload + + @pytest.mark.parametrize( ("tool", "arguments"), [ From ee1afedcdd8aadfa61bfa163cfb7e327bb28e0e1 Mon Sep 17 00:00:00 2001 From: Sophie Song Date: Fri, 25 Sep 2026 12:49:30 -0700 Subject: [PATCH 6/8] fix(announcements): harden authoring client lifecycle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19335911-b012-4b7f-b179-e4caeae7ea69 --- .../src/mcp/agentconfig_core/base_client.py | 24 +- .../agentconfig_org_announcements/server.py | 480 +++++++++++------- .../test_authoring_client_lifecycle.py | 108 +++- .../test_mcp_app_protocol.py | 46 +- 4 files changed, 444 insertions(+), 214 deletions(-) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_core/base_client.py b/solutions/ess-maker-skills/src/mcp/agentconfig_core/base_client.py index e7f7b9bd2..8fd7c6f07 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_core/base_client.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_core/base_client.py @@ -96,7 +96,7 @@ class _CredentialFailure(Enum): ) -class _LocalCredentialError(ValueError): +class LocalCredentialError(ValueError): """Local validation failure with an allowlisted renewal diagnostic.""" def __init__(self, message: str, reason: _CredentialFailure): @@ -121,18 +121,18 @@ def _resolve_token() -> str: def _read_token_file(token_file: str) -> str: if not os.path.isfile(token_file): - raise _LocalCredentialError( + raise LocalCredentialError( "AGENTCONFIG_ACCESS_TOKEN_FILE does not exist", _CredentialFailure.FILE_MISSING ) try: with open(token_file, "r", encoding="utf-8") as handle: token = handle.read().strip() except (OSError, UnicodeError) as error: - raise _LocalCredentialError( + raise LocalCredentialError( "AGENTCONFIG_ACCESS_TOKEN_FILE could not be read", _CredentialFailure.FILE_UNREADABLE ) from error if not token: - raise _LocalCredentialError( + raise LocalCredentialError( "AGENTCONFIG_ACCESS_TOKEN_FILE is empty", _CredentialFailure.FILE_EMPTY ) return token @@ -143,7 +143,7 @@ def _validate_token_tenant(token: str, expected_tenant_id: str | None) -> str: expected_tenant_id is not None and _decode_tenant_id_from_jwt(token) != expected_tenant_id ): - raise _LocalCredentialError( + raise LocalCredentialError( "The AgentConfiguration token tenant does not match the configured " "Dataverse environment. Sign in to that tenant or provide a matching token.", _CredentialFailure.TENANT_MISMATCH, @@ -292,7 +292,7 @@ def _decode_jwt_payload(token: str) -> dict[str, Any]: """ parts = token.split(".") if len(parts) != 3: - raise _LocalCredentialError( + raise LocalCredentialError( "AGENTCONFIG_ACCESS_TOKEN does not look like a JWT " "(expected three dot-separated segments)", _CredentialFailure.INVALID_TOKEN, @@ -302,12 +302,12 @@ def _decode_jwt_payload(token: str) -> dict[str, Any]: try: payload = json.loads(base64.urlsafe_b64decode(padded)) except ValueError as error: - raise _LocalCredentialError( + raise LocalCredentialError( "Could not decode AGENTCONFIG_ACCESS_TOKEN payload", _CredentialFailure.INVALID_TOKEN, ) from error if not isinstance(payload, dict): - raise _LocalCredentialError( + raise LocalCredentialError( "AGENTCONFIG_ACCESS_TOKEN payload must be an object", _CredentialFailure.INVALID_TOKEN ) return payload @@ -317,13 +317,13 @@ def _decode_tenant_id_from_jwt(token: str) -> str: """Decode and validate the tenant ID (``tid``) used to address the route.""" tenant_id = _decode_jwt_payload(token).get("tid") if not isinstance(tenant_id, str) or not tenant_id: - raise _LocalCredentialError( + raise LocalCredentialError( "AGENTCONFIG_ACCESS_TOKEN payload has no 'tid' claim", _CredentialFailure.INVALID_TOKEN ) try: return str(uuid.UUID(tenant_id)) except ValueError as error: - raise _LocalCredentialError( + raise LocalCredentialError( "AGENTCONFIG_ACCESS_TOKEN payload has an invalid 'tid' claim", _CredentialFailure.INVALID_TOKEN ) from error @@ -352,7 +352,7 @@ def validate_token_identity(token: str, tenant_id: str, object_id: str | None) - raise ValueError("The authoring account cannot be identified without an oid claim") actual = _decode_object_id_from_jwt(token) if actual is None or actual.casefold() != object_id.casefold(): - raise _LocalCredentialError( + raise LocalCredentialError( "The token does not identify the intended authoring account", _CredentialFailure.ACCOUNT_MISMATCH ) return token @@ -467,7 +467,7 @@ async def _refresh_token(self, rejected_token: str) -> None: "Could not renew credentials for the original account. " "Provide matching credentials and retry." ) - if isinstance(error, _LocalCredentialError): + if isinstance(error, LocalCredentialError): message = error.reason.value elif isinstance(error, LockException): message = ( diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py index 5dba81174..320b3b825 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -50,6 +50,7 @@ build_manager_state, is_deleted_item, ) +from base_client import LocalCredentialError from drafts import ( AnnouncementEditorDraft, AudienceGroup, @@ -282,6 +283,8 @@ def _widget_shell() -> str: ) _client: Optional[OrgAnnouncementsClient] = None +_client_users: dict[OrgAnnouncementsClient, int] = {} +_retired_clients: set[OrgAnnouncementsClient] = set() _graph_client: Optional[GraphDirectoryClient] = None _graph_client_users: dict[GraphDirectoryClient, int] = {} @@ -291,7 +294,7 @@ def _widget_shell() -> str: async def get_client() -> OrgAnnouncementsClient: - """Return the authoring client, constructing it lazily off the event loop. + """Lease the authoring client, constructing it lazily off the event loop. ``OrgAnnouncementsClient.__init__`` resolves a delegated token through the shared core, which is synchronous and — on a cold cache — blocks for as long @@ -299,27 +302,58 @@ async def get_client() -> OrgAnnouncementsClient: inline would run all of that *inside* the asyncio event loop, freezing every other in-flight request and the MCP stdio transport itself for the duration. - So construction happens in a worker thread, behind a lock, exactly like the - Graph client's token acquisition. + Construction and lease registration happen behind the same lock. Callers + must release the returned client, normally through ``get_client_lease``, so + a 401 reset can retire the client without closing it during another request. """ global _client - if _client is not None: - return _client async with _client_lock: if _client is None: try: _client = await asyncio.to_thread(OrgAnnouncementsClient) - except (LockException, OSError) as error: + except (LocalCredentialError, LockException, OSError) as error: # Cache details stay on the exception cause, not in tool payloads. raise _FailureResult( "AuthenticationRequired", "Organization announcement sign-in could not be completed. Try again.", source=SOURCE_MCP, ) from error - return _client + client = _client + _client_users[client] = _client_users.get(client, 0) + 1 + return client -async def reset_client() -> None: +async def release_client(client: OrgAnnouncementsClient) -> None: + """Release one authoring-client lease and close a retired final user.""" + close_client = None + async with _client_lock: + users = _client_users.get(client) + if users is None: + return + if users > 1: + _client_users[client] = users - 1 + return + del _client_users[client] + if client in _retired_clients: + _retired_clients.remove(client) + close_client = client + if close_client is not None: + await close_client.aclose() + + +@asynccontextmanager +async def get_client_lease() -> AsyncIterator[OrgAnnouncementsClient]: + """Keep one authoring client alive for the complete tool operation.""" + client = await get_client() + try: + yield client + finally: + await release_client(client) + + +async def reset_client( + failed_client: Optional[OrgAnnouncementsClient] = None, +) -> bool: """Drop the authoring client so the next call reauthenticates. The shared core resolves its token once, in ``__init__``, and holds it for @@ -329,13 +363,27 @@ async def reset_client() -> None: Dropping the object is therefore the reauthentication mechanism: the next ``get_client()`` builds a fresh one and re-runs silent-first MSAL, which normally refreshes from the shared cache with no prompt. + + ``failed_client`` makes invalidation compare-and-swap: a delayed 401 from a + retired client cannot evict a healthy replacement. Active users retain a + lease, so the retired client closes only after its final operation exits. """ global _client + close_client = None async with _client_lock: - client, _client = _client, None - # Closed outside the lock so teardown never blocks a concurrent rebuild. - if client is not None: - await client.aclose() + client = _client + if client is None or ( + failed_client is not None and failed_client is not client + ): + return False + _client = None + if _client_users.get(client): + _retired_clients.add(client) + else: + close_client = client + if close_client is not None: + await close_client.aclose() + return True @asynccontextmanager @@ -794,8 +842,16 @@ def org_announcements_widget() -> str: ) async def list_agent_configs() -> str: """List configured deployed ESS agents; never initialize configuration.""" - client = await get_client() - return json.dumps(await client.list_agent_configs(), indent=2) + try: + async with get_client_lease() as client: + try: + result = await client.list_agent_configs() + except AgentConfigApiError as error: + failure = await _failure_from(error, client) + raise ToolError(failure.message) from None + except _FailureResult as failure: + raise ToolError(failure.message) from None + return json.dumps(result, indent=2) @mcp.tool( @@ -803,8 +859,16 @@ async def list_agent_configs() -> str: ) async def search_agents(searchString: str) -> str: """Find deployed ESS agents by name to resolve their titleId.""" - client = await get_client() - return json.dumps(await client.search_agents(searchString), indent=2) + try: + async with get_client_lease() as client: + try: + result = await client.search_agents(searchString) + except AgentConfigApiError as error: + failure = await _failure_from(error, client) + raise ToolError(failure.message) from None + except _FailureResult as failure: + raise ToolError(failure.message) from None + return json.dumps(result, indent=2) @mcp.tool( @@ -849,6 +913,7 @@ async def open_org_announcements( started = time.monotonic() scope = {"titleId": titleId} + client = None request = validated.model_dump(exclude_none=True, exclude={"suggestedDraft"}) if suggestedDraft is not None: request["suggestedDraft"] = suggestedDraft.retry_payload() @@ -856,53 +921,68 @@ async def open_org_announcements( try: # Capture once even for an empty create. Subsequent reads and post-save # refreshes must never adopt a different global client's tenant. - client = await get_client() - scope = {"tenantId": client.tenant_id, "titleId": titleId} + async with get_client_lease() as client: + scope = {"tenantId": client.tenant_id, "titleId": titleId} + + if view == "manager": + async with get_graph_client( + scope["tenantId"], client.object_id + ) as graph_client: + state = await _manager_state(client, scope, graph_client) + return _open_success( + {"view": "manager", **state}, + "Opened announcements for the selected ESS agent.", + started, + ) - if view == "manager": - async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: - state = await _manager_state(client, scope, graph_client) - return _open_success( - {"view": "manager", **state}, - "Opened announcements for the selected ESS agent.", - started, - ) + if mode == "create": + async with get_graph_client( + scope["tenantId"], client.object_id + ) as graph_client: + audience_metadata = await _resolve_audience_metadata( + graph_client, + list(suggestedDraft.audience) + if suggestedDraft is not None and suggestedDraft.audience + else [], + ) + draft = build_create_draft(suggestedDraft, audience_metadata) + return _open_success( + _editor_payload("create", None, draft, scope), + "Opened a new organization announcement for review. Nothing is " + "saved until you publish or save a draft in the editor.", + started, + ) - if mode == "create": - async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: + config = await client.get_bulletin(titleId, bulletinId) + audience = config.get("audience") + async with get_graph_client( + scope["tenantId"], client.object_id + ) as graph_client: audience_metadata = await _resolve_audience_metadata( graph_client, - list(suggestedDraft.audience) - if suggestedDraft is not None and suggestedDraft.audience + [ + group_id + for group_id in audience + if isinstance(group_id, str) + ] + if isinstance(audience, list) else [], ) - draft = build_create_draft(suggestedDraft, audience_metadata) + draft = build_editor_draft_from_config(config, audience_metadata) + message = "Opened the announcement for editing." return _open_success( - _editor_payload("create", None, draft, scope), - "Opened a new organization announcement for review. Nothing is " - "saved until you publish or save a draft in the editor.", - started, + _editor_payload("edit", config, draft, scope), message, started ) - config = await client.get_bulletin(titleId, bulletinId) - audience = config.get("audience") - async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: - audience_metadata = await _resolve_audience_metadata( - graph_client, - [group_id for group_id in audience if isinstance(group_id, str)] - if isinstance(audience, list) - else [], - ) - draft = build_editor_draft_from_config(config, audience_metadata) - message = "Opened the announcement for editing." - return _open_success( - _editor_payload("edit", config, draft, scope), message, started - ) - except _FailureResult as failure: return _open_failure(request, failure, started, scope) except AgentConfigApiError as error: - return _open_failure(request, await _failure_from(error), started, scope) + return _open_failure( + request, + await _failure_from(error, client), + started, + scope, + ) except httpx.RequestError: return _open_failure( request, @@ -947,7 +1027,10 @@ def _mutation_failure( ) -async def _failure_from(error: Exception) -> _FailureResult: +async def _failure_from( + error: Exception, + authoring_client: Optional[OrgAnnouncementsClient], +) -> _FailureResult: """Classify a failure and reauthenticate the authoring client on a 401. A 401 from the authoring API means the cached delegated token is expired or @@ -962,7 +1045,7 @@ async def _failure_from(error: Exception) -> _FailureResult: """ failure = _to_failure(error) if failure.code == "AuthenticationRequired" and failure.source == SOURCE_BACKEND: - await reset_client() + await reset_client(authoring_client) return failure @@ -1019,7 +1102,9 @@ async def _saved_item_result( ) manager = await _manager_state(client, scope, graph_client) except _MUTATION_ERRORS as error: - raise _CommittedRefreshError(await _failure_from(error)) from error + raise _CommittedRefreshError( + await _failure_from(error, client) + ) from error return _text_result( { @@ -1072,44 +1157,49 @@ async def save_bulletin( ) payload = request.model_dump(mode="json", exclude_none=True) + client = None try: - client = await get_client() - scope = {"tenantId": client.tenant_id, "titleId": titleId} - saved = await client.save_bulletin(titleId, payload) + async with get_client_lease() as client: + scope = {"tenantId": client.tenant_id, "titleId": titleId} + saved = await client.save_bulletin(titleId, payload) + + try: + result = await _saved_item_result( + client, + scope, + saved, + "Published the organization announcement." + if status == "published" + else "Saved the organization announcement draft.", + ) + except _CommittedRefreshError as error: + # The write is committed. Report the explicit partial success so the + # widget tells the maker to refresh instead of offering a retry that + # would create a second announcement. + _LOGGER.warning( + "save_bulletin refresh failed after commit: %s", + error.cause.code, + ) + return _fail( + "save_bulletin", + _committed_refresh_failure(error.cause), + started, + scope, + ) + + record_operation( + "save_bulletin", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + return result except _MUTATION_ERRORS as error: - failure = await _failure_from(error) + failure = await _failure_from(error, client) _LOGGER.warning( "save_bulletin failed: %s (create=%s)", failure.code, id is None ) return _fail("save_bulletin", failure, started, scope) - try: - result = await _saved_item_result( - client, - scope, - saved, - "Published the organization announcement." - if status == "published" - else "Saved the organization announcement draft.", - ) - except _CommittedRefreshError as error: - # The write is committed. Report the explicit partial success so the - # widget tells the maker to refresh instead of offering a retry that - # would create a second announcement. - _LOGGER.warning( - "save_bulletin refresh failed after commit: %s", error.cause.code - ) - return _fail( - "save_bulletin", _committed_refresh_failure(error.cause), started, scope - ) - - record_operation( - "save_bulletin", - outcome="success", - latency_ms=_elapsed_ms(started), - ) - return result - @mcp.tool( meta=_app_only_tool_meta(), @@ -1143,56 +1233,64 @@ async def transition_bulletin( """ started = time.monotonic() scope = {"titleId": titleId} + client = None try: validate_title_id(titleId) validate_bulletin_id(id) - client = await get_client() - scope = {"tenantId": client.tenant_id, "titleId": titleId} - changed = await client.transition_bulletin( - titleId, id, TRANSITION_STATUS[transition] - ) + async with get_client_lease() as client: + scope = {"tenantId": client.tenant_id, "titleId": titleId} + changed = await client.transition_bulletin( + titleId, id, TRANSITION_STATUS[transition] + ) + + # The transition is committed from here on. A refresh failure must never be + # reported as a retryable normal failure, because the lifecycle change has + # already been applied and re-issuing it could fail validation or move the + # record again. + try: + async with get_graph_client( + scope["tenantId"], client.object_id + ) as graph_client: + manager = await _manager_state(client, scope, graph_client) + except _MUTATION_ERRORS as error: + cause = await _failure_from(error, client) + _LOGGER.warning( + "transition_bulletin refresh failed after commit: %s (%s)", + cause.code, + transition, + ) + return _fail( + "transition_bulletin", + _committed_refresh_failure(cause), + started, + scope, + ) + + record_operation( + "transition_bulletin", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + # The canonical changed row is included alongside the manager state. It is + # additive: the widget's existing manager-shaped contract is untouched, so a + # host that strips unknown fields simply ignores ``item`` and still gets a + # correct refresh. + return _text_result( + { + **scope, + "status": "success", + "item": {"config": changed}, + "manager": manager, + }, + "Updated the organization announcement.", + ) except _MUTATION_ERRORS as error: - failure = await _failure_from(error) + failure = await _failure_from(error, client) _LOGGER.warning( "transition_bulletin failed: %s (%s)", failure.code, transition ) return _fail("transition_bulletin", failure, started, scope) - # The transition is committed from here on. A refresh failure must never be - # reported as a retryable normal failure, because the lifecycle change has - # already been applied and re-issuing it could fail validation or move the - # record again. - try: - async with get_graph_client(scope["tenantId"], client.object_id) as graph_client: - manager = await _manager_state(client, scope, graph_client) - except _MUTATION_ERRORS as error: - cause = await _failure_from(error) - _LOGGER.warning( - "transition_bulletin refresh failed after commit: %s (%s)", - cause.code, - transition, - ) - return _fail( - "transition_bulletin", _committed_refresh_failure(cause), started, scope - ) - - record_operation( - "transition_bulletin", - outcome="success", - latency_ms=_elapsed_ms(started), - ) - # The canonical changed row is included alongside the manager state. It is - # additive: the widget's existing manager-shaped contract is untouched, so a - # host that strips unknown fields simply ignores ``item`` and still gets a - # correct refresh. - return _text_result( - { - **scope, "status": "success", - "item": {"config": changed}, "manager": manager, - }, - "Updated the organization announcement.", - ) - @mcp.tool( meta=_app_only_tool_meta(), @@ -1211,68 +1309,77 @@ async def duplicate_bulletin( """ started = time.monotonic() scope = {"titleId": titleId} + client = None try: validate_title_id(titleId) validate_bulletin_id(id) - client = await get_client() - scope = {"tenantId": client.tenant_id, "titleId": titleId} - source = await client.get_bulletin(titleId, id) - except _MUTATION_ERRORS as error: - failure = await _failure_from(error) - _LOGGER.warning("duplicate_bulletin source load failed: %s", failure.code) - return _fail("duplicate_bulletin", failure, started, scope) + async with get_client_lease() as client: + scope = {"tenantId": client.tenant_id, "titleId": titleId} + source = await client.get_bulletin(titleId, id) + + # Stored content is forwarded verbatim, so it never passes through + # BulletinInput. The blank-schedule sentinel is stripped explicitly here for + # the same reason it is coerced there: "" is not a DateTimeOffset, and + # sending it would fail model binding on a copy the maker never edited. + bulletin = without_blank_schedule( + { + key: value + for key, value in source["bulletin"].items() + if key not in _COPY_STRIPPED_FIELDS + } + ) + audience = source.get("audience") + payload = { + "bulletin": bulletin, + "audience": [ + group_id + for group_id in ( + audience if isinstance(audience, list) else [] + ) + if isinstance(group_id, str) + ], + "status": "draft", + } - # Stored content is forwarded verbatim, so it never passes through - # BulletinInput. The blank-schedule sentinel is stripped explicitly here for - # the same reason it is coerced there: "" is not a DateTimeOffset, and - # sending it would fail model binding on a copy the maker never edited. - bulletin = without_blank_schedule( - { - key: value - for key, value in source["bulletin"].items() - if key not in _COPY_STRIPPED_FIELDS - } - ) - audience = source.get("audience") - payload = { - "bulletin": bulletin, - "audience": [ - group_id - for group_id in (audience if isinstance(audience, list) else []) - if isinstance(group_id, str) - ], - "status": "draft", - } + try: + created = await client.save_bulletin(titleId, payload) + except _MUTATION_ERRORS as error: + failure = await _failure_from(error, client) + _LOGGER.warning("duplicate_bulletin failed: %s", failure.code) + return _fail("duplicate_bulletin", failure, started, scope) - try: - created = await client.save_bulletin(titleId, payload) + try: + result = await _saved_item_result( + client, + scope, + created, + "Created a draft copy of the organization announcement.", + ) + except _CommittedRefreshError as error: + # The copy exists. This is an unkeyed create, so a retry would produce a + # second copy; report the committed partial success instead. + _LOGGER.warning( + "duplicate_bulletin refresh failed after commit: %s", + error.cause.code, + ) + return _fail( + "duplicate_bulletin", + _committed_refresh_failure(error.cause), + started, + scope, + ) + + record_operation( + "duplicate_bulletin", + outcome="success", + latency_ms=_elapsed_ms(started), + ) + return result except _MUTATION_ERRORS as error: - failure = await _failure_from(error) - _LOGGER.warning("duplicate_bulletin failed: %s", failure.code) + failure = await _failure_from(error, client) + _LOGGER.warning("duplicate_bulletin source load failed: %s", failure.code) return _fail("duplicate_bulletin", failure, started, scope) - try: - result = await _saved_item_result( - client, scope, created, - "Created a draft copy of the organization announcement.", - ) - except _CommittedRefreshError as error: - # The copy exists. This is an unkeyed create, so a retry would produce a - # second copy; report the committed partial success instead. - _LOGGER.warning( - "duplicate_bulletin refresh failed after commit: %s", error.cause.code - ) - return _fail( - "duplicate_bulletin", _committed_refresh_failure(error.cause), started, scope - ) - - record_operation( - "duplicate_bulletin", - outcome="success", - latency_ms=_elapsed_ms(started), - ) - return result - @mcp.tool( meta=_shared_tool_meta(), @@ -1291,15 +1398,16 @@ async def search_audience_groups(query: str) -> CallToolResult: group does not exist when it simply was not reached. """ started = time.monotonic() + authoring_client = None try: escape_search_value(query) - authoring_client = await get_client() - async with get_graph_client( - authoring_client.tenant_id, authoring_client.object_id - ) as graph_client: - result = await graph_client.search_groups(query) + async with get_client_lease() as authoring_client: + async with get_graph_client( + authoring_client.tenant_id, authoring_client.object_id + ) as graph_client: + result = await graph_client.search_groups(query) except (_FailureResult, GraphDirectoryError, AgentConfigApiError, httpx.RequestError) as error: - failure = await _failure_from(error) + failure = await _failure_from(error, authoring_client) _LOGGER.warning("search_audience_groups failed: %s", failure.code) record_operation( "search_audience_groups", diff --git a/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py b/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py index 382a9d002..03e6e83c8 100644 --- a/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py +++ b/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py @@ -40,8 +40,12 @@ def _reset_globals(): """Never leak a fake client between tests or into another module.""" org_server._client = None + org_server._client_users.clear() + org_server._retired_clients.clear() yield org_server._client = None + org_server._client_users.clear() + org_server._retired_clients.clear() class _FakeAuthoringClient: @@ -84,7 +88,8 @@ def _construct() -> _FakeAuthoringClient: async def run() -> int: loop_thread = threading.get_ident() - await org_server.get_client() + client = await org_server.get_client() + await org_server.release_client(client) return loop_thread loop_thread = asyncio.run(run()) @@ -106,6 +111,8 @@ async def run() -> None: first = await org_server.get_client() second = await org_server.get_client() assert first is second + await org_server.release_client(first) + await org_server.release_client(second) asyncio.run(run()) @@ -123,11 +130,14 @@ def _construct() -> _FakeAuthoringClient: monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) async def run() -> list: - return await asyncio.gather( + clients = await asyncio.gather( org_server.get_client(), org_server.get_client(), org_server.get_client(), ) + for client in clients: + await org_server.release_client(client) + return clients clients = asyncio.run(run()) @@ -140,15 +150,17 @@ async def run() -> list: # -------------------------------------------------------------------------- -def test_reset_drops_and_closes_the_authoring_client(monkeypatch) -> None: +def test_reset_defers_close_until_the_final_lease_is_released(monkeypatch) -> None: monkeypatch.setattr( org_server, "OrgAnnouncementsClient", _FakeAuthoringClient ) async def run() -> _FakeAuthoringClient: client = await org_server.get_client() - await org_server.reset_client() + assert await org_server.reset_client(client) assert org_server._client is None + assert not client.closed + await org_server.release_client(client) return client client = asyncio.run(run()) @@ -173,9 +185,12 @@ def _construct() -> _FakeAuthoringClient: async def run() -> None: first = await org_server.get_client() - await org_server.reset_client() + assert await org_server.reset_client(first) second = await org_server.get_client() assert first is not second + assert not first.closed + await org_server.release_client(first) + await org_server.release_client(second) asyncio.run(run()) @@ -191,10 +206,13 @@ def test_an_authoring_401_resets_the_client(monkeypatch) -> None: async def run() -> None: client = await org_server.get_client() failure = await org_server._failure_from( - org_client.AgentConfigApiError("HttpError: HTTP 401", http_status=401) + org_client.AgentConfigApiError("HttpError: HTTP 401", http_status=401), + client, ) assert failure.code == "AuthenticationRequired" assert org_server._client is None + assert not client.closed + await org_server.release_client(client) assert client.closed asyncio.run(run()) @@ -215,11 +233,12 @@ def test_a_graph_401_does_not_reset_the_authoring_client(monkeypatch) -> None: async def run() -> None: client = await org_server.get_client() - failure = await org_server._failure_from(graph_error) + failure = await org_server._failure_from(graph_error, client) assert failure.code == "AuthenticationRequired" assert failure.source == org_server.SOURCE_GRAPH assert org_server._client is client assert not client.closed + await org_server.release_client(client) asyncio.run(run()) @@ -235,22 +254,89 @@ async def run() -> None: await org_server._failure_from( org_client.AgentConfigApiError( f"HttpError: HTTP {status}", http_status=status - ) + ), + client, ) assert org_server._client is client assert not client.closed + await org_server.release_client(client) asyncio.run(run()) def test_resetting_when_no_client_exists_is_a_no_op() -> None: async def run() -> None: - await org_server.reset_client() + assert not await org_server.reset_client() assert org_server._client is None asyncio.run(run()) +def test_reset_does_not_close_a_client_used_by_another_operation(monkeypatch) -> None: + built: list[_FakeAuthoringClient] = [] + + def _construct() -> _FakeAuthoringClient: + client = _FakeAuthoringClient() + built.append(client) + return client + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + + async def run() -> None: + failed_client = await org_server.get_client() + overlapping_client = await org_server.get_client() + assert failed_client is overlapping_client + + assert await org_server.reset_client(failed_client) + replacement = await org_server.get_client() + + assert replacement is not failed_client + assert not failed_client.closed + await org_server.release_client(failed_client) + assert not failed_client.closed + await org_server.release_client(overlapping_client) + assert failed_client.closed + assert not replacement.closed + await org_server.release_client(replacement) + + asyncio.run(run()) + + assert len(built) == 2 + + +def test_a_stale_401_cannot_evict_a_replacement_client(monkeypatch) -> None: + built: list[_FakeAuthoringClient] = [] + + def _construct() -> _FakeAuthoringClient: + client = _FakeAuthoringClient() + built.append(client) + return client + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + unauthorized = org_client.AgentConfigApiError( + "HttpError: HTTP 401", http_status=401 + ) + + async def run() -> None: + old_first = await org_server.get_client() + old_second = await org_server.get_client() + await org_server._failure_from(unauthorized, old_first) + + replacement = await org_server.get_client() + await org_server._failure_from(unauthorized, old_second) + + assert org_server._client is replacement + assert not replacement.closed + await org_server.release_client(old_first) + await org_server.release_client(old_second) + assert old_first.closed + await org_server.release_client(replacement) + + asyncio.run(run()) + + assert len(built) == 2 + + # -------------------------------------------------------------------------- # stdout purity # -------------------------------------------------------------------------- @@ -378,7 +464,9 @@ def test_constructing_the_authoring_client_writes_nothing_to_stdout( async def run() -> Any: with contextlib.redirect_stdout(captured_out): - return await org_server.get_client() + client = await org_server.get_client() + await org_server.release_client(client) + return client client = asyncio.run(run()) diff --git a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py index 82f76cb0f..7a0fa2240 100644 --- a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py +++ b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py @@ -2415,6 +2415,10 @@ def test_directory_tenant_binding_does_not_add_mcp_arguments() -> None: _PRIVATE_CACHE_DETAIL = "/private/msal-cache PRIVATE_CREDENTIAL_DETAIL" +def _local_credential_error(message: str) -> Exception: + return org_server.LocalCredentialError(message, object()) + + def _install_failing_graph_acquisition(monkeypatch, error, *, before_failure=None): directory = org_server.GraphDirectoryClient(tenant_id=TENANT_ID, object_id=OBJECT_ID) acquisitions = [] @@ -2521,7 +2525,10 @@ def test_graph_cache_failure_preserves_open_and_search_envelopes( _assert_no_private_cache_detail(result, caplog) -@pytest.mark.parametrize("error_type", [LockException, PermissionError]) +@pytest.mark.parametrize( + "error_factory", + [LockException, PermissionError, _local_credential_error], +) @pytest.mark.parametrize( ("tool", "arguments"), [ @@ -2533,13 +2540,13 @@ def test_graph_cache_failure_preserves_open_and_search_envelopes( ], ) def test_authoring_cache_failure_preserves_feature_envelopes( - monkeypatch, caplog, error_type, tool, arguments + monkeypatch, caplog, error_factory, tool, arguments ) -> None: attempts = [] def construct(): attempts.append(True) - raise error_type(_PRIVATE_CACHE_DETAIL) + raise error_factory(_PRIVATE_CACHE_DETAIL) monkeypatch.setattr(org_server, "_client", None) monkeypatch.setattr(org_server, "OrgAnnouncementsClient", construct) @@ -2562,9 +2569,14 @@ def construct(): _assert_no_private_cache_detail(result, caplog) -@pytest.mark.parametrize("error_type", [LockException, PermissionError]) -def test_authoring_cache_failure_preserves_its_private_cause(monkeypatch, error_type) -> None: - original = error_type(_PRIVATE_CACHE_DETAIL) +@pytest.mark.parametrize( + "error_factory", + [LockException, PermissionError, _local_credential_error], +) +def test_authoring_cache_failure_preserves_its_private_cause( + monkeypatch, error_factory +) -> None: + original = error_factory(_PRIVATE_CACHE_DETAIL) def construct(): raise original @@ -2580,3 +2592,25 @@ async def run(): assert _PRIVATE_CACHE_DETAIL not in caught.value.message asyncio.run(run()) + + +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("list_agent_configs", {}), + ("search_agents", {"searchString": "Finance"}), + ], +) +def test_discovery_credential_failure_is_a_safe_tool_error( + monkeypatch, tool, arguments +) -> None: + def construct(): + raise _local_credential_error(_PRIVATE_CACHE_DETAIL) + + monkeypatch.setattr(org_server, "_client", None) + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", construct) + + with pytest.raises(ToolError, match="sign-in could not be completed") as caught: + _call(tool, arguments, include_scope=False) + + assert _PRIVATE_CACHE_DETAIL not in str(caught.value) From 25a410f03af75dffac2aa572fe98d9b191dbc14c Mon Sep 17 00:00:00 2001 From: Sophie Song Date: Fri, 25 Sep 2026 13:34:31 -0700 Subject: [PATCH 7/8] fix(announcements): keep backend codes out of diagnostics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19335911-b012-4b7f-b179-e4caeae7ea69 --- .../agentconfig_org_announcements/server.py | 51 ++++++++++++++----- .../telemetry.py | 30 +++++++++-- .../test_telemetry_privacy.py | 41 ++++++++++++--- 3 files changed, 97 insertions(+), 25 deletions(-) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py index 320b3b825..abd86b427 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -72,6 +72,7 @@ SOURCE_BACKEND, SOURCE_GRAPH, SOURCE_MCP, + normalize_error_code, record_operation, ) from validation import validate_bulletin_id, validate_title_id @@ -164,6 +165,9 @@ "NotFound": "The announcement was not found.", "ServiceError": "The Org Announcements service could not complete the request.", } +_BACKEND_DIAGNOSTIC_CODES = frozenset( + {*_MODEL_VISIBLE_BACKEND_MESSAGES, "CommittedRefreshFailed"} +) # Identity and audit fields the backend owns. A duplicate strips them from the # copied content so the copy is created as a fresh Draft rather than silently @@ -491,6 +495,16 @@ def as_error_list(self) -> list[dict[str, Any]]: return [self.as_error()] +def _diagnostic_code(failure: _FailureResult) -> str: + """Return a content-free code for logs and telemetry.""" + if ( + failure.source == SOURCE_BACKEND + and failure.code not in _BACKEND_DIAGNOSTIC_CODES + ): + return "BackendValidationError" + return normalize_error_code(failure.code) + + def _validation_failure(error: BulletinValidationError) -> _FailureResult: """Adapt an HTTP-200 ``EssBulletinSaveResult`` rejection. @@ -814,12 +828,13 @@ def _open_failure( started: float, scope: dict[str, str], ) -> CallToolResult: - _LOGGER.warning("open_org_announcements failed: %s", failure.code) + diagnostic_code = _diagnostic_code(failure) + _LOGGER.warning("open_org_announcements failed: %s", diagnostic_code) record_operation( "open_org_announcements", outcome="failure", latency_ms=_elapsed_ms(started), - error_code=failure.code, + error_code=diagnostic_code, error_source=failure.source, ) return _open_error_payload(request, failure, scope) @@ -1005,11 +1020,12 @@ def _fail( operation: str, failure: _FailureResult, started: float, scope: dict[str, str] ) -> CallToolResult: """Emit the content-free failure event and build the tool result.""" + diagnostic_code = _diagnostic_code(failure) record_operation( operation, outcome="failure", latency_ms=_elapsed_ms(started), - error_code=failure.code, + error_code=diagnostic_code, error_source=failure.source, ) return _mutation_failure(failure, scope) @@ -1178,7 +1194,7 @@ async def save_bulletin( # would create a second announcement. _LOGGER.warning( "save_bulletin refresh failed after commit: %s", - error.cause.code, + _diagnostic_code(error.cause), ) return _fail( "save_bulletin", @@ -1196,7 +1212,9 @@ async def save_bulletin( except _MUTATION_ERRORS as error: failure = await _failure_from(error, client) _LOGGER.warning( - "save_bulletin failed: %s (create=%s)", failure.code, id is None + "save_bulletin failed: %s (create=%s)", + _diagnostic_code(failure), + id is None, ) return _fail("save_bulletin", failure, started, scope) @@ -1256,7 +1274,7 @@ async def transition_bulletin( cause = await _failure_from(error, client) _LOGGER.warning( "transition_bulletin refresh failed after commit: %s (%s)", - cause.code, + _diagnostic_code(cause), transition, ) return _fail( @@ -1287,7 +1305,9 @@ async def transition_bulletin( except _MUTATION_ERRORS as error: failure = await _failure_from(error, client) _LOGGER.warning( - "transition_bulletin failed: %s (%s)", failure.code, transition + "transition_bulletin failed: %s (%s)", + _diagnostic_code(failure), + transition, ) return _fail("transition_bulletin", failure, started, scope) @@ -1345,7 +1365,10 @@ async def duplicate_bulletin( created = await client.save_bulletin(titleId, payload) except _MUTATION_ERRORS as error: failure = await _failure_from(error, client) - _LOGGER.warning("duplicate_bulletin failed: %s", failure.code) + _LOGGER.warning( + "duplicate_bulletin failed: %s", + _diagnostic_code(failure), + ) return _fail("duplicate_bulletin", failure, started, scope) try: @@ -1360,7 +1383,7 @@ async def duplicate_bulletin( # second copy; report the committed partial success instead. _LOGGER.warning( "duplicate_bulletin refresh failed after commit: %s", - error.cause.code, + _diagnostic_code(error.cause), ) return _fail( "duplicate_bulletin", @@ -1377,7 +1400,10 @@ async def duplicate_bulletin( return result except _MUTATION_ERRORS as error: failure = await _failure_from(error, client) - _LOGGER.warning("duplicate_bulletin source load failed: %s", failure.code) + _LOGGER.warning( + "duplicate_bulletin source load failed: %s", + _diagnostic_code(failure), + ) return _fail("duplicate_bulletin", failure, started, scope) @@ -1408,12 +1434,13 @@ async def search_audience_groups(query: str) -> CallToolResult: result = await graph_client.search_groups(query) except (_FailureResult, GraphDirectoryError, AgentConfigApiError, httpx.RequestError) as error: failure = await _failure_from(error, authoring_client) - _LOGGER.warning("search_audience_groups failed: %s", failure.code) + diagnostic_code = _diagnostic_code(failure) + _LOGGER.warning("search_audience_groups failed: %s", diagnostic_code) record_operation( "search_audience_groups", outcome="failure", latency_ms=_elapsed_ms(started), - error_code=failure.code, + error_code=diagnostic_code, error_source=failure.source, ) return CallToolResult( diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py index 15bce2f31..8929a973f 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py @@ -58,6 +58,22 @@ # Stable codes are short identifiers, never free text. This bound is a # belt-and-braces guard so a malformed code can never carry a payload. _MAX_CODE_LENGTH = 64 +_ERROR_CODES = frozenset( + { + "AudienceMetadataUnavailable", + "AuthenticationRequired", + "AuthorizationDenied", + "BackendValidationError", + "CommittedRefreshFailed", + "FeatureUnavailable", + "IndeterminateWrite", + "InvalidRequest", + "NetworkError", + "NotFound", + "SearchUnavailable", + "ServiceError", + } +) # Emitting is opt-out through the same switch the rest of the ADK honours; the # module-level import is resolved lazily so a server started outside the kit @@ -87,18 +103,22 @@ def normalize_source(source: str) -> str: def normalize_error_code(error_code: str) -> str: - """Keep only a short, identifier-shaped stable code. + """Clamp an error code to the fixed diagnostic allowlist. - A backend code arrives as an identifier such as ``AudienceGroupInvalid``. - Anything containing whitespace or punctuation is a message, not a code, so - it is replaced rather than truncated — a truncated message is still content. + Backend values can be caller-controlled even when they look like short + identifiers, so shape validation alone is not a privacy boundary. """ if not isinstance(error_code, str): return "" candidate = error_code.strip() if not candidate: return "" - if len(candidate) > _MAX_CODE_LENGTH or not candidate.replace("_", "").isalnum(): + if ( + len(candidate) > _MAX_CODE_LENGTH + or not candidate.isascii() + or not candidate.replace("_", "").isalnum() + or candidate not in _ERROR_CODES + ): return "UnknownError" return candidate diff --git a/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py b/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py index 3a5553959..a2edf823d 100644 --- a/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py +++ b/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py @@ -14,6 +14,7 @@ from __future__ import annotations import asyncio +import logging import sys from contextlib import asynccontextmanager from datetime import datetime, timezone @@ -122,7 +123,7 @@ def test_a_failure_event_adds_only_a_stable_code_and_broad_source( "save_bulletin", outcome="failure", latency_ms=7, - error_code="AudienceRequired", + error_code="BackendValidationError", error_source=org_telemetry.SOURCE_BACKEND, ) @@ -131,7 +132,7 @@ def test_a_failure_event_adds_only_a_stable_code_and_broad_source( "api_endpoint": "save_bulletin", "outcome": "failure", "latency_ms": 7, - "error_code": "AudienceRequired", + "error_code": "BackendValidationError", "error_category": "backend", # Never a message: backend text can echo the announcement back. "error_message": "", @@ -188,11 +189,13 @@ def test_an_unknown_error_source_is_bucketed(emitted) -> None: @pytest.mark.parametrize( ("raw", "expected"), [ - ("AudienceRequired", "AudienceRequired"), - ("Committed_Refresh_Failed", "Committed_Refresh_Failed"), + ("AuthenticationRequired", "AuthenticationRequired"), + ("CommittedRefreshFailed", "CommittedRefreshFailed"), ("", ""), # A message masquerading as a code is replaced, not truncated: a # truncated message is still content. + ("SecretProject", "UnknownError"), + ("秘密", "UnknownError"), ("Announcement 'Layoffs' is invalid", "UnknownError"), ("https://contoso.example/x", "UnknownError"), ("a" * 200, "UnknownError"), @@ -202,6 +205,22 @@ def test_only_identifier_shaped_error_codes_are_emitted(raw, expected) -> None: assert org_telemetry.normalize_error_code(raw) == expected +def test_backend_diagnostic_codes_are_allowlisted() -> None: + committed = org_server._FailureResult( + "CommittedRefreshFailed", + "Saved, but refresh failed.", + source=org_telemetry.SOURCE_BACKEND, + ) + caller_controlled = org_server._FailureResult( + "SecretProject", + "Rejected.", + source=org_telemetry.SOURCE_BACKEND, + ) + + assert org_server._diagnostic_code(committed) == "CommittedRefreshFailed" + assert org_server._diagnostic_code(caller_controlled) == "BackendValidationError" + + def test_a_negative_latency_is_clamped(emitted) -> None: org_telemetry.record_operation( "save_bulletin", outcome="success", latency_ms=-5 @@ -375,14 +394,17 @@ def test_a_successful_save_emits_no_content_or_group_identifier( assert secret not in flat, f"{secret!r} leaked into telemetry" -def test_a_failed_save_emits_only_the_stable_code(monkeypatch, emitted) -> None: +def test_a_failed_save_keeps_the_backend_code_out_of_diagnostics( + monkeypatch, emitted, caplog +) -> None: + caller_controlled_code = "SecretProject" _install_fakes( monkeypatch, client=_FakeClient( save_error=org_client.BulletinValidationError( [ { - "code": "AudienceGroupInvalid", + "code": caller_controlled_code, "field": "audience", # A backend message can echo content straight back. "message": f"Group {SECRET_GROUP_ID} is invalid for " @@ -393,12 +415,15 @@ def test_a_failed_save_emits_only_the_stable_code(monkeypatch, emitted) -> None: ), ) - _call("save_bulletin", _save_arguments()) + with caplog.at_level(logging.WARNING, logger="ess-org-announcements"): + payload = _call("save_bulletin", _save_arguments()) assert emitted event = emitted[-1] - assert event["error_code"] == "AudienceGroupInvalid" + assert event["error_code"] == "BackendValidationError" assert event["error_message"] == "" + assert payload["errors"][0]["code"] == caller_controlled_code + assert caller_controlled_code not in caplog.text for secret in FORBIDDEN: assert secret not in _flatten(event) From 8b9a5465d68f74347f6755085164db9e183efd6c Mon Sep 17 00:00:00 2001 From: Sophie Song Date: Fri, 25 Sep 2026 16:12:23 -0700 Subject: [PATCH 8/8] fix(announcements): harden client lifecycle and telemetry Make authoring construction and cleanup cancellation-safe, sanitize model-visible discovery failures, and cover every registered tool with privacy-safe telemetry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19335911-b012-4b7f-b179-e4caeae7ea69 --- .../requirements.txt | 1 + .../agentconfig_org_announcements/server.py | 230 ++++++++++++------ .../telemetry.py | 2 + .../test_authoring_client_lifecycle.py | 189 ++++++++++++++ .../test_mcp_app_protocol.py | 60 +++++ .../test_telemetry_privacy.py | 39 ++- 6 files changed, 450 insertions(+), 71 deletions(-) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/requirements.txt b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/requirements.txt index 0d0198932..dcc23755e 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/requirements.txt +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/requirements.txt @@ -1,4 +1,5 @@ mcp>=1.29.0,<2.0.0 +anyio>=4.0,<5.0 httpx>=0.27.0,<1.0 msal>=1.35.0 pydantic>=2.0,<3.0 diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py index abd86b427..165e5926d 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/server.py @@ -35,6 +35,7 @@ from typing import Any, Literal, Optional from urllib.parse import urlsplit +import anyio import httpx from mcp.server.fastmcp import FastMCP from mcp.server.fastmcp.exceptions import ToolError @@ -287,6 +288,7 @@ def _widget_shell() -> str: ) _client: Optional[OrgAnnouncementsClient] = None +_client_construction_task: Optional[asyncio.Task[None]] = None _client_users: dict[OrgAnnouncementsClient, int] = {} _retired_clients: set[OrgAnnouncementsClient] = set() _graph_client: Optional[GraphDirectoryClient] = None @@ -297,6 +299,50 @@ def _widget_shell() -> str: _client_lock = asyncio.Lock() +def _observe_task_failure(task: asyncio.Task[None]) -> None: + """Retrieve an unobserved construction failure after all waiters cancel.""" + if not task.cancelled(): + task.exception() + + +async def _construct_and_publish_client() -> None: + """Build one authoring client outside the lock and publish or dispose it.""" + global _client, _client_construction_task + current_task = asyncio.current_task() + client = None + published = False + try: + try: + client = await asyncio.to_thread(OrgAnnouncementsClient) + except (LocalCredentialError, LockException, OSError) as error: + raise _FailureResult( + "AuthenticationRequired", + "Organization announcement sign-in could not be completed. Try again.", + source=SOURCE_MCP, + ) from error + + with anyio.CancelScope(shield=True): + close_client = None + async with _client_lock: + if _client is None: + _client = client + published = True + else: + close_client = client + if close_client is not None: + await close_client.aclose() + except asyncio.CancelledError: + if client is not None and not published: + with anyio.CancelScope(shield=True): + await client.aclose() + raise + finally: + with anyio.CancelScope(shield=True): + async with _client_lock: + if _client_construction_task is current_task: + _client_construction_task = None + + async def get_client() -> OrgAnnouncementsClient: """Lease the authoring client, constructing it lazily off the event loop. @@ -306,43 +352,52 @@ async def get_client() -> OrgAnnouncementsClient: inline would run all of that *inside* the asyncio event loop, freezing every other in-flight request and the MCP stdio transport itself for the duration. - Construction and lease registration happen behind the same lock. Callers - must release the returned client, normally through ``get_client_lease``, so - a 401 reset can retire the client without closing it during another request. + One shared construction task is registered behind the lock, but the + potentially human-duration authentication itself runs outside it. Lease + registration remains lock-protected. Callers must release the returned + client, normally through ``get_client_lease``, so a 401 reset can retire the + client without closing it during another request. """ - global _client - async with _client_lock: - if _client is None: - try: - _client = await asyncio.to_thread(OrgAnnouncementsClient) - except (LocalCredentialError, LockException, OSError) as error: - # Cache details stay on the exception cause, not in tool payloads. - raise _FailureResult( - "AuthenticationRequired", - "Organization announcement sign-in could not be completed. Try again.", - source=SOURCE_MCP, - ) from error - client = _client - _client_users[client] = _client_users.get(client, 0) + 1 - return client + global _client_construction_task + while True: + async with _client_lock: + if _client is not None: + client = _client + _client_users[client] = _client_users.get(client, 0) + 1 + return client + construction = _client_construction_task + if construction is None: + construction = asyncio.create_task(_construct_and_publish_client()) + construction.add_done_callback(_observe_task_failure) + _client_construction_task = construction + try: + await asyncio.shield(construction) + except asyncio.CancelledError: + current = asyncio.current_task() + if construction.cancelled() and ( + current is None or not current.cancelling() + ): + continue + raise async def release_client(client: OrgAnnouncementsClient) -> None: """Release one authoring-client lease and close a retired final user.""" - close_client = None - async with _client_lock: - users = _client_users.get(client) - if users is None: - return - if users > 1: - _client_users[client] = users - 1 - return - del _client_users[client] - if client in _retired_clients: - _retired_clients.remove(client) - close_client = client - if close_client is not None: - await close_client.aclose() + with anyio.CancelScope(shield=True): + close_client = None + async with _client_lock: + users = _client_users.get(client) + if users is None: + return + if users > 1: + _client_users[client] = users - 1 + return + del _client_users[client] + if client in _retired_clients: + _retired_clients.remove(client) + close_client = client + if close_client is not None: + await close_client.aclose() @asynccontextmanager @@ -417,16 +472,18 @@ async def get_graph_client( and previous is not client and not _graph_client_users.get(previous) ): - await previous.aclose() + with anyio.CancelScope(shield=True): + await previous.aclose() yield client finally: - remaining = _graph_client_users[client] - 1 - if remaining: - _graph_client_users[client] = remaining - else: - del _graph_client_users[client] - if client is not _graph_client: - await client.aclose() + with anyio.CancelScope(shield=True): + remaining = _graph_client_users[client] - 1 + if remaining: + _graph_client_users[client] = remaining + else: + del _graph_client_users[client] + if client is not _graph_client: + await client.aclose() def _now() -> datetime: @@ -771,6 +828,25 @@ def _editor_payload( } +def _model_visible_failure(failure: _FailureResult) -> _FailureResult: + """Replace backend-controlled detail before exposing a failure to the model.""" + if failure.source == SOURCE_BACKEND: + safe_message = _MODEL_VISIBLE_BACKEND_MESSAGES.get(failure.code) + if safe_message is None: + return _FailureResult( + "InvalidRequest", + "The Org Announcements service rejected the request.", + source=SOURCE_BACKEND, + ) + return _FailureResult( + failure.code, + safe_message, + retryable=failure.retryable, + source=SOURCE_BACKEND, + ) + return failure + + def _open_error_payload( request: dict[str, Any], failure: _FailureResult, scope: dict[str, str] ) -> CallToolResult: @@ -779,22 +855,7 @@ def _open_error_payload( The request is response-only retry state, including the original suggestion. It is never logged or sent to telemetry. """ - visible_failure = failure - if failure.source == SOURCE_BACKEND: - safe_message = _MODEL_VISIBLE_BACKEND_MESSAGES.get(failure.code) - if safe_message is None: - visible_failure = _FailureResult( - "InvalidRequest", - "The Org Announcements service rejected the request.", - source=SOURCE_BACKEND, - ) - else: - visible_failure = _FailureResult( - failure.code, - safe_message, - retryable=failure.retryable, - source=SOURCE_BACKEND, - ) + visible_failure = _model_visible_failure(failure) payload = { **scope, @@ -840,6 +901,25 @@ def _open_failure( return _open_error_payload(request, failure, scope) +async def _discovery_tool_error( + operation: str, + error: Exception, + started: float, + client: Optional[OrgAnnouncementsClient], +) -> ToolError: + failure = _model_visible_failure(await _failure_from(error, client)) + diagnostic_code = _diagnostic_code(failure) + _LOGGER.warning("%s failed: %s", operation, diagnostic_code) + record_operation( + operation, + outcome="failure", + latency_ms=_elapsed_ms(started), + error_code=diagnostic_code, + error_source=failure.source, + ) + return ToolError(failure.message) + + @mcp.resource( ORG_ANNOUNCEMENTS_RESOURCE_URI, name="Org announcements", @@ -857,15 +937,20 @@ def org_announcements_widget() -> str: ) async def list_agent_configs() -> str: """List configured deployed ESS agents; never initialize configuration.""" + started = time.monotonic() + client = None try: async with get_client_lease() as client: - try: - result = await client.list_agent_configs() - except AgentConfigApiError as error: - failure = await _failure_from(error, client) - raise ToolError(failure.message) from None - except _FailureResult as failure: - raise ToolError(failure.message) from None + result = await client.list_agent_configs() + except (_FailureResult, AgentConfigApiError, httpx.RequestError) as error: + raise await _discovery_tool_error( + "list_agent_configs", error, started, client + ) from None + record_operation( + "list_agent_configs", + outcome="success", + latency_ms=_elapsed_ms(started), + ) return json.dumps(result, indent=2) @@ -874,15 +959,20 @@ async def list_agent_configs() -> str: ) async def search_agents(searchString: str) -> str: """Find deployed ESS agents by name to resolve their titleId.""" + started = time.monotonic() + client = None try: async with get_client_lease() as client: - try: - result = await client.search_agents(searchString) - except AgentConfigApiError as error: - failure = await _failure_from(error, client) - raise ToolError(failure.message) from None - except _FailureResult as failure: - raise ToolError(failure.message) from None + result = await client.search_agents(searchString) + except (_FailureResult, AgentConfigApiError, httpx.RequestError) as error: + raise await _discovery_tool_error( + "search_agents", error, started, client + ) from None + record_operation( + "search_agents", + outcome="success", + latency_ms=_elapsed_ms(started), + ) return json.dumps(result, indent=2) diff --git a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py index 8929a973f..65d12b210 100644 --- a/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py +++ b/solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/telemetry.py @@ -39,8 +39,10 @@ # new dimension value (and cannot smuggle caller-controlled text into Aria). _OPERATIONS = frozenset( { + "list_agent_configs", "open_org_announcements", "save_bulletin", + "search_agents", "transition_bulletin", "duplicate_bulletin", "search_audience_groups", diff --git a/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py b/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py index 03e6e83c8..d94091cb1 100644 --- a/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py +++ b/tests/mcp/agentconfig_org_announcements/test_authoring_client_lifecycle.py @@ -22,6 +22,7 @@ from pathlib import Path from typing import Any +import anyio import pytest @@ -40,12 +41,20 @@ def _reset_globals(): """Never leak a fake client between tests or into another module.""" org_server._client = None + org_server._client_construction_task = None org_server._client_users.clear() org_server._retired_clients.clear() + org_server._client_lock = asyncio.Lock() + org_server._graph_client = None + org_server._graph_client_users.clear() yield org_server._client = None + org_server._client_construction_task = None org_server._client_users.clear() org_server._retired_clients.clear() + org_server._client_lock = asyncio.Lock() + org_server._graph_client = None + org_server._graph_client_users.clear() class _FakeAuthoringClient: @@ -145,6 +154,99 @@ async def run() -> list: assert clients[0] is clients[1] is clients[2] +def test_cancelled_first_waiter_does_not_duplicate_construction( + monkeypatch, +) -> None: + """Request cancellation must not cancel shared interactive sign-in.""" + started = threading.Event() + unblock = threading.Event() + built: list[_FakeAuthoringClient] = [] + + def _construct() -> _FakeAuthoringClient: + client = _FakeAuthoringClient() + built.append(client) + started.set() + assert unblock.wait(timeout=5) + return client + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + + async def run() -> None: + first_scope = anyio.CancelScope() + second_clients: list[_FakeAuthoringClient] = [] + + async def first_waiter() -> None: + with first_scope: + await org_server.get_client() + + async def second_waiter() -> None: + second_clients.append(await org_server.get_client()) + + async with anyio.create_task_group() as task_group: + task_group.start_soon(first_waiter) + with anyio.fail_after(5): + await anyio.to_thread.run_sync(started.wait) + first_scope.cancel() + await anyio.sleep(0) + task_group.start_soon(second_waiter) + await anyio.sleep(0.05) + assert len(built) == 1 + unblock.set() + + assert len(second_clients) == 1 + assert org_server._client is second_clients[0] is built[0] + await org_server.release_client(second_clients[0]) + assert org_server._client_users == {} + + anyio.run(run, backend="asyncio") + + assert len(built) == 1 + assert not built[0].closed + + +def test_cancelled_construction_is_disposed_and_retried(monkeypatch) -> None: + """A cancelled shared task cannot wedge later uncancelled callers.""" + first_started = threading.Event() + first_unblock = threading.Event() + built: list[_FakeAuthoringClient] = [] + + def _construct() -> _FakeAuthoringClient: + client = _FakeAuthoringClient() + built.append(client) + if len(built) == 1: + first_started.set() + assert first_unblock.wait(timeout=5) + return client + + monkeypatch.setattr(org_server, "OrgAnnouncementsClient", _construct) + + async def run() -> None: + acquired: list[_FakeAuthoringClient] = [] + + async def waiter() -> None: + acquired.append(await org_server.get_client()) + + async with anyio.create_task_group() as task_group: + task_group.start_soon(waiter) + with anyio.fail_after(5): + await anyio.to_thread.run_sync(first_started.wait) + async with org_server._client_lock: + first_unblock.set() + await anyio.sleep(0.05) + construction = org_server._client_construction_task + assert construction is not None + construction.cancel() + + assert len(built) == 2 + assert built[0].closed + assert acquired == [built[1]] + assert org_server._client is built[1] + assert org_server._client_construction_task is None + await org_server.release_client(acquired[0]) + + anyio.run(run, backend="asyncio") + + # -------------------------------------------------------------------------- # Reauthentication after a 401 # -------------------------------------------------------------------------- @@ -168,6 +270,93 @@ async def run() -> _FakeAuthoringClient: assert client.closed, "the stale client was dropped without being closed" +def test_cancelled_operation_still_releases_a_retired_client( + monkeypatch, +) -> None: + """FastMCP's AnyIO cancellation cannot interrupt lease bookkeeping.""" + client = _FakeAuthoringClient() + monkeypatch.setattr(org_server, "_client", client) + + async def run() -> None: + leased = anyio.Event() + scope_ready = anyio.Event() + scopes: list[anyio.CancelScope] = [] + + async def operation() -> None: + with anyio.CancelScope() as scope: + scopes.append(scope) + scope_ready.set() + async with org_server.get_client_lease(): + leased.set() + await anyio.sleep_forever() + + async def retire_while_holding_lock() -> None: + await leased.wait() + await scope_ready.wait() + async with org_server._client_lock: + org_server._client = None + org_server._retired_clients.add(client) + scopes[0].cancel() + await anyio.sleep(0.05) + + async with anyio.create_task_group() as task_group: + task_group.start_soon(operation) + task_group.start_soon(retire_while_holding_lock) + + assert client not in org_server._client_users + assert client not in org_server._retired_clients + assert client.closed + + anyio.run(run, backend="asyncio") + + +def test_cancelled_graph_operation_closes_a_replaced_client() -> None: + """Graph cleanup follows the same cancellation-safe resource rule.""" + + class _FakeGraphClient: + def __init__(self, name: str) -> None: + self.name = name + self.tenant_id = "tenant" + self.object_id = "object" + self.closed = False + + async def aclose(self) -> None: + self.closed = True + + original = _FakeGraphClient("original") + replacement = _FakeGraphClient("replacement") + org_server._graph_client = original + + async def run() -> None: + leased = anyio.Event() + scope_ready = anyio.Event() + scopes: list[anyio.CancelScope] = [] + + async def operation() -> None: + with anyio.CancelScope() as scope: + scopes.append(scope) + scope_ready.set() + async with org_server.get_graph_client("tenant", "object"): + leased.set() + await anyio.sleep_forever() + + async def replace_and_cancel() -> None: + await leased.wait() + await scope_ready.wait() + org_server._graph_client = replacement + scopes[0].cancel() + + async with anyio.create_task_group() as task_group: + task_group.start_soon(operation) + task_group.start_soon(replace_and_cancel) + + assert original not in org_server._graph_client_users + assert original.closed + assert not replacement.closed + + anyio.run(run, backend="asyncio") + + def test_the_call_after_a_reset_builds_a_fresh_client(monkeypatch) -> None: """Rebuilding is the reauthentication mechanism. diff --git a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py index 7a0fa2240..16fb5d89a 100644 --- a/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py +++ b/tests/mcp/agentconfig_org_announcements/test_mcp_app_protocol.py @@ -2614,3 +2614,63 @@ def construct(): _call(tool, arguments, include_scope=False) assert _PRIVATE_CACHE_DETAIL not in str(caught.value) + + +@pytest.mark.parametrize( + ("tool", "arguments", "status"), + [ + ("list_agent_configs", {}, 400), + ("search_agents", {"searchString": "Finance"}, 422), + ], +) +def test_discovery_backend_failure_is_a_safe_tool_error( + fake_clients, tool, arguments, status +) -> None: + private_marker = "SECRET_BACKEND_MARKER" + + class _FailingDiscoveryClient(_FakeClient): + async def list_agent_configs(self): + raise org_client.AgentConfigApiError( + f"PrivateCode: {private_marker}", + http_status=status, + ) + + async def search_agents(self, search_string): + raise org_client.AgentConfigApiError( + f"PrivateCode: {private_marker}", + http_status=status, + ) + + fake_clients(client=_FailingDiscoveryClient()) + + with pytest.raises(ToolError, match="service rejected the request") as caught: + _call(tool, arguments, include_scope=False) + + assert private_marker not in str(caught.value) + + +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("list_agent_configs", {}), + ("search_agents", {"searchString": "Finance"}), + ], +) +def test_discovery_transport_failure_is_a_safe_tool_error( + fake_clients, tool, arguments +) -> None: + private_marker = "https://private-backend.example/secret" + + class _FailingDiscoveryClient(_FakeClient): + async def list_agent_configs(self): + raise httpx.ConnectError(private_marker) + + async def search_agents(self, search_string): + raise httpx.ConnectError(private_marker) + + fake_clients(client=_FailingDiscoveryClient()) + + with pytest.raises(ToolError, match="temporarily unavailable") as caught: + _call(tool, arguments, include_scope=False) + + assert private_marker not in str(caught.value) diff --git a/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py b/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py index a2edf823d..5e714f210 100644 --- a/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py +++ b/tests/mcp/agentconfig_org_announcements/test_telemetry_privacy.py @@ -152,8 +152,10 @@ def test_the_endpoint_dimension_is_the_tool_name_not_a_url(emitted) -> None: @pytest.mark.parametrize( "operation", [ + "list_agent_configs", "open_org_announcements", "save_bulletin", + "search_agents", "transition_bulletin", "duplicate_bulletin", "search_audience_groups", @@ -174,6 +176,12 @@ def test_an_unknown_operation_is_bucketed_rather_than_emitted(emitted) -> None: assert emitted[0]["api_endpoint"] == org_telemetry.OPERATION_UNKNOWN +def test_every_registered_tool_has_an_observability_policy() -> None: + tools = {tool.name for tool in asyncio.run(org_server.mcp.list_tools())} + + assert tools == org_telemetry._OPERATIONS + + def test_an_unknown_error_source_is_bucketed(emitted) -> None: org_telemetry.record_operation( "save_bulletin", @@ -306,6 +314,12 @@ class _FakeClient: def __init__(self, *, save_error: Exception | None = None) -> None: self.save_error = save_error + async def list_agent_configs(self) -> list[dict]: + return [{"titleId": TITLE_ID, "displayName": "Secret agent"}] + + async def search_agents(self, search_string: str) -> list[dict]: + return [{"titleId": TITLE_ID, "displayName": "Secret agent"}] + async def list_bulletins(self, title_id: str) -> list[dict]: return [_config()] @@ -354,7 +368,11 @@ async def _get_graph_client(tenant_id, object_id): def _call(tool: str, arguments: dict) -> dict: - if tool != "search_audience_groups": + if tool not in { + "list_agent_configs", + "search_agents", + "search_audience_groups", + }: arguments = {"titleId": TITLE_ID, **arguments} async def run(): return await org_server.mcp.call_tool(tool, arguments) @@ -467,6 +485,25 @@ def test_search_telemetry_never_carries_the_query(monkeypatch, emitted) -> None: assert SECRET_TITLE not in _flatten(event) +@pytest.mark.parametrize( + ("tool", "arguments"), + [ + ("list_agent_configs", {}), + ("search_agents", {"searchString": SECRET_TITLE}), + ], +) +def test_discovery_telemetry_records_success_without_input( + monkeypatch, emitted, tool, arguments +) -> None: + _install_fakes(monkeypatch) + + _call(tool, arguments) + + assert emitted[-1]["api_endpoint"] == tool + assert emitted[-1]["outcome"] == "success" + assert SECRET_TITLE not in _flatten(emitted[-1]) + + def test_a_transition_emits_no_identifier(monkeypatch, emitted) -> None: _install_fakes(monkeypatch)