You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Expose the separate announcements MCP provider, widget resource, manager/editor opener, audience lookup, and widget-owned authoring operations.
Current diff
Keep flat opener arguments: manager requires titleId; create requires explicit create intent without an existing bulletinId; edit requires bulletinId. Reject malformed combinations before generating widget retry state.
Open incomplete and repairable copies as ordinary ID-less create editors with config: null, retaining supplied content and localized copy titles. Opening never saves, publishes, transitions, or duplicates.
Preserve captured tenant/account directory leases, scoped retry context, and non-repeating recovery for indeterminate or committed-but-unrefreshed writes.
Preserve isError: true with operation-specific structured failure payloads. CommittedRefreshFailed remains non-retryable and retains the actual refresh cause without fabricating a saved ID, item, or manager result.
Restrict the trusted Vorpal widget origin to the known dev, DF, and production hosts. Loopback and *.devtunnels.ms origins require the explicit VORPAL_WIDGET_ALLOW_DEVELOPMENT_ORIGIN=1 development gate; arbitrary HTTPS and misleading suffix hosts remain rejected.
Share the public title/bulletin request validators between the client and server. Bulletin IDs reject URL dot-segments, route-shaping delimiters, encoded separators, control characters, whitespace changes, and excessive length before route construction. Every mutation validates a supplied ID before authentication and client acquisition.
Add cross-provider discovery/import-isolation coverage and retain existing privacy-safe feature telemetry. No additional announcements client-event bridge is introduced.
Feature contract
The opener and discovery tools are read-only. The widget owns mutations after opening. Editable copies use the create opener; their first explicit Save establishes identity. The app-only immediate-write duplicate_bulletin tool and persisted Unarchive transition remain unchanged. Unknown ownership and malformed retry requests are not accepted. Opening content for repair does not make its supplied actions valid to save or publish.
The paired Vorpal consumer catches McpToolError and returns error.result, preserving the structured MCP result for isError: true responses. This keeps the non-retryable CommittedRefreshFailed guidance available to the widget instead of reducing it to a generic thrown error.
Stack and dependency
This is slice 4/5. Head: users/rebova/org-announcements-review-runtime. Base: users/rebova/org-announcements-prerelease after #271 merged on September 25, 2026. The future release target remains TBD and its final promotion baseline must be confirmed separately.
Bootstrap prerequisite #262 merged into release/planner-landing-page on September 10, 2026. This stack remains pinned to cacb1bec056428809f1ccb0383561190d516bee4, which is an ancestor of merge commit 8a04f5f40f334e729a3497877edca655730f1be2. Unchanged prerequisite work is excluded from this slice.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Runtime file guide.
This informational walkthrough maps all eight changed files. Runtime here connects agent/widget requests to the authoring and directory clients from PRs #270–#271; it is not UI layout or backend storage. Toolkit startup and entry-point routing belong to PR #273.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Include the runtime checks in CI.
The announcements job now selects the whole feature test directory instead of naming three individual files. That includes the new protocol, client-lifecycle, and telemetry tests alongside the earlier client, draft, and directory tests, without adding a workflow entry for each file. The foundation job also gains test_import_isolation.py, which checks that the sibling MCP providers load their own modules. These changes expand test selection; they do not start or deploy the announcements service.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Connect requests to announcement services.
This file connects requests to PR #270's authoring client and PR #271's Graph client—not screen layout or storage.
org_announcements_widget returns a static HTML shell loading Vorpal. Metadata exposes read tools to the model/app and mutations only to the app.
open_org_announcements takes flat titleId/view arguments. Manager has no mode; editor requires create or edit, with an ID only for edit. Pydantic checks combinations before widget-error handling: malformed calls become ToolError; valid failed opens retain the original request without inventing an ID/mode. Opening never saves.
Each operation captures its authoring client and authenticated tenant alongside titleId; directory work also binds the account. get_graph_client counts active users so replacing a tenant/account client does not close it mid-operation.
save_bulletin sends complete content/audience. transition_bulletin delegates identifier/status-only changes; publishing uses a full save, not a republish mode. duplicate_bulletin strips identity/audit fields and creates a Draft before any later editor opening, preserving its title and leaving the source unchanged.
_saved_item_result combines acknowledged content with refreshed manager/audience data. IndeterminateWrite and CommittedRefreshFailed are non-retryable results: inspect current state instead of blindly repeating a create/copy. A manager refresh does not guarantee identifying the saved record.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Measure operations without recording authored content.
record_operation adapts the runtime's calls to the existing adk.api.call emitter; it is not a separate widget-event bridge. The api_endpoint field holds an allowlisted tool name, not the service URL. Success adds outcome and duration; failure also adds a short identifier-shaped error code and a broad backend/Graph/MCP category, with an empty error message. Unknown operation/source values are replaced, and message-shaped or overlong codes become UnknownError. Error codes are shape-checked, not a fixed allowlist. The helper resolves the emitter lazily and tolerates its absence or emission exceptions, keeping those telemetry failures separate from the tool result.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Share discovery without coupling the feature providers.
Adding announcements to the existing parameterized tests applies the same discovery contract to both clients: shared helper methods, preserved titleId values, expected request routes, rejected invalid searches, and errors for malformed collections rather than empty successes. The feature-tool check uses a discovery-only fake, so listing/searching agents does not require configuration initialization or attach a widget. Announcements awaits its client and advertises readOnlyHint; the landing-page provider keeps its existing synchronous accessor and unchanged metadata. Search uses a POST request here, but its purpose is still to find existing agents, not create configuration.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Keep sign-in from blocking the request transport.
These tests cover how the server obtains and replaces its authoring client, rather than announcement content. Fakes check that construction happens off the async event-loop thread, concurrent first requests share one construction, and reset drops and closes the old client so a subsequent call can build another. Authoring-side 401 failures trigger that reset; Graph authentication failures and other tested service statuses do not. Mocked sign-in paths also check that notices go to stderr, not stdout, because stdout carries MCP protocol messages. Additional guards look for stdout prints and overly verbose HTTP logging. No real browser sign-in is needed for these scenarios.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Exercise the contract the host and widget call.
_call goes through FastMCP's tool dispatcher, while fake authoring/directory clients record requests and inject failures. This covers resource metadata, tool visibility, structured results, complete saves, minimal transitions, duplicate creation, and audience lookup. The schema assertions intentionally require only titleId and view globally; separate invocation tests check the conditional manager/create/edit rules and ToolError boundary. Valid failed opens retain retry context instead. Other cases distinguish uncertain writes from acknowledged writes whose refresh fails, checking that the mutation is not repeated. Scope and overlapping-request cases check captured tenant/agent/account context and Graph-client replacement. These are host/runtime contract checks with simulated dependencies, not evidence about rendered UI or deployed backend behavior.
The reason will be displayed to describe this comment to others. Learn more.
Walkthrough — Prevent sibling servers from importing each other's code.
Both MCP providers have files named client.py and server.py. Loading them under those plain module names in one Python process can make the second provider reuse the first provider's module. These tests check the shared isolated loader returns distinct clients from the correct directories, reuses each provider's own modules on repeated loads, and restores previous plain-name entries. A source scan discourages direct flat imports in test modules; a separate subprocess checks that both providers coexist in one interpreter. This protects the test harness from misleading cross-provider failures, not tenant isolation or live service routing.
Commit 50ced4d documents repairable copy opening and adds regressions for zero-write hydration, priority and unresolved-audience handling, backend action rejections, and exactly-one-write committed-refresh failure packaging. The mutation handlers and failure envelope are unchanged; dependency updates were merged forward from the existing contracts/audience branches.
[copilot-review] Re-verified at head d46e9c15. Verdict: Approve with should-fixes (none blocking). This supersedes my earlier review at f77eb747: the follow-up commit "Harden announcement runtime error handling" resolved the two model-visible leaks below, with tests. Only two authors have commented on this PR (the author's file walkthroughs and this review), so there is no external reviewer feedback to reconcile.
Design verdict
Right layer for a 4/5 runtime slice. Verified directly: open_org_announcements is read-only and model/app-visible; save_bulletin, transition_bulletin, and duplicate_bulletin are app-only via _app_only_tool_meta(); the model-visible opener rejects bad manager/editor argument combinations before any I/O; tenant and title scope come from the authoring client, never caller input; Graph $search and $filter are both injection-safe (escape_search_value plus _is_safe_filter_id); committed-but-unrefreshed writes are correctly non-retryable so an unkeyed create or duplicate cannot be replayed into a second record (_MUTATION_ERRORS includes both _FailureResult and GraphDirectoryError, so a Graph failure during post-commit refresh still yields the committed, non-retryable result rather than an unhandled error).
RESOLVED — model-visible backend detail leak. _open_error_payload now rebuilds model-visible failures from a local _MODEL_VISIBLE_BACKEND_MESSAGES map and collapses unknown backend codes to a generic InvalidRequest, so backend detail / str(error) no longer reaches the model. Covered by new tests (400/409/422 sanitization, recognized-code rebuild).
RESOLVED — validation input echo. StrictModel now sets hide_input_in_errors=True, so save_bulletin (and every request model) no longer echoes the maker's title/description/action URL/group IDs on a ValidationError. Covered by new tests in test_drafts.py and test_mcp_app_protocol.py.
Still open (posted as inline comments)
Should-fix (defense in depth): _resolve_widget_origin accepts any HTTPS VORPAL_WIDGET_ORIGIN; restrict to an explicit allowlist. See inline on server.py:170.
Nit: private validators _validate_title_id / _validate_bulletin_id imported from client across the MCP request boundary; promote to a shared public module. See inline on server.py:52.
Question: confirm the real MCP app host preserves structuredContent on isError=True for CommittedRefreshFailed; if it drops it, the "saved, do not repeat" guidance is lost. See inline on server.py:558.
What is good
Content-free telemetry and logs (stable codes only, never content, query, group IDs, tokens, or claims); group IDs never rendered as display names; allowlisted MSAL error codes before logging; careful async locking so concurrent tool calls share one sign-in; follow_redirects=False on the Graph client; Graph error strings are static (no query text or group IDs), so the unsanitized Graph-source path to the model is safe; and meaningful tests across mutation visibility, committed-refresh failures, audience hydration and search, Graph auth and cache failures, import isolation, telemetry privacy, and the new sanitization paths.
Verification
Read server.py, drafts.py, graph_directory_client.py, telemetry.py, the hardening commit patch, and the new tests at head d46e9c15; confirmed the sanitization logic, hide_input_in_errors, static Graph messages, and _MUTATION_ERRORS coverage myself. Did NOT run tests locally (blocked by an ARM64 cryptography wheel build failure); the author reports 601 offline tests pass on Python 3.13, and Python 3.11 CI is still unchecked in the PR checklist. Coverage gaps: no local test execution, and no live MCP app host / Graph / backend validation. No new commits or external comments since this review.
Initial credential-file/token validation failures are raised by the shared core as _LocalCredentialError, a ValueError, not as OSError/LockException. This handler lets them escape to the opener/mutation ValueError paths as InvalidRequest (and the discovery handlers can expose an unhandled tool exception), so a missing, malformed, or mismatched credential is not reported as the structured AuthenticationRequired failure that the widget can act on. Normalize credential-resolution failures here before returning the client.
Follow-up for the previously-missed credential finding in the latest Copilot overview: fixed in ee1afed. The shared core now exposes the precise LocalCredentialError type, and the announcements server normalizes that type alongside lock/OS credential failures instead of broadly catching ValueError. Protocol regressions cover missing, unreadable, empty, malformed, tenant-mismatched, and account-mismatched local credentials; opener/mutations preserve the structured AuthenticationRequired envelope, discovery returns a safe tool error, and private credential/cache details are not exposed. Final local validation: 651 Org Announcements tests and 306 shared/foundation tests passed (1 expected Windows permission-bit skip), plus Ruff, compile, and git diff --check.
The reason will be displayed to describe this comment to others. Learn more.
Three follow-up findings against 25a410f: model-visible discovery error sanitization and two cancellation-related client-lifecycle issues. Each was reproduced against the pinned runtime; these are distinct from the previously resolved threads.
The reason will be displayed to describe this comment to others. Learn more.
Important: sanitize backend errors on model-visible discovery tools.list_agent_configs forwards failure.message directly into ToolError, and search_agents does the same below. For HTTP 400/409/422, _classify_api_error preserves the backend Message verbatim, so these model-visible tools bypass the sanitization applied by _open_error_payload. I reproduced this with an HTTP 400 containing a synthetic private marker: both discovery tool errors exposed it, while the opener correctly returned the local generic message. Please share the model-facing error-message policy across discovery and the opener, keeping raw backend validation detail confined to app-only mutation results, and add regression coverage for both discovery tools.
The reason will be displayed to describe this comment to others. Learn more.
Important: make lease release cancellation-safe. This cleanup awaits release_client() inside the request cancellation scope. If client A has been retired and another request is constructing its replacement while holding _client_lock, cancelling A's operation under MCP's AnyIO CancelScope interrupts cleanup at the lock acquisition before its lease count is decremented. I reproduced A remaining in _retired_clients with one recorded user and never being closed after the cancelled operation and replacement construction had both finished. Please shield the release bookkeeping and teardown from request cancellation, and avoid holding the bookkeeping lock throughout potentially human-duration authentication. A regression should use AnyIO cancellation with a contended lock; a single plain Task.cancel() does not reproduce the same cleanup behavior.
The reason will be displayed to describe this comment to others. Learn more.
Important: preserve shared construction across request cancellation. Cancelling this await releases _client_lock, but asyncio.to_thread does not stop the running constructor. _client stays unset, so the next request starts a second constructor while the first browser sign-in may still be pending, and the first constructor's eventual result is abandoned. I reproduced two simultaneously active constructors by cancelling the first acquisition before unblocking authentication. Please track construction in a shared task/future whose lifetime is independent of any one request, shield individual waiters appropriately, and ensure the eventual result is either published or disposed of. Add cancellation coverage alongside the existing concurrent-first-request test.
1. Model-visible backend detail leak on the discovery tools._classify_api_error copies the backend detail into failure.message for 400/409/422, and list_agent_configs / search_agents both do raise ToolError(failure.message) — bypassing the _MODEL_VISIBLE_BACKEND_MESSAGES sanitization that _open_error_payload applies. This is the same leak class that was fixed for the opener, still open on the other two model-visible tools. test_discovery_credential_failure_is_a_safe_tool_error covers only the credential construction path, not an AgentConfigApiError raised from the request. Suggest routing both through the same sanitizer and adding a 400/422 regression.
2. Discovery tools are absent from telemetry.list_agent_configs and search_agents are not in telemetry._OPERATIONS and call neither record_operation nor _LOGGER.warning. Every other tool records success and failure, so discovery failures are invisible on the dashboard. If that's deliberate, a short comment in _OPERATIONS would prevent it reading as an omission.
3. Cold sign-in can block unrelated lease releases.get_client correctly runs the blocking MSAL construction via asyncio.to_thread, but it holds _client_lock for the whole interactive prompt, and release_client needs that same lock. After a 401 retires a client that still has active leases, an unrelated in-flight tool call can block on its own release until a human finishes the browser sign-in. Constructing outside the lock and re-checking under it, or a separate lease-accounting lock, would remove the coupling.
Nits
transition_bulletin uses _DESTRUCTIVE_ANNOTATIONS for all four transitions, but only delete is destructive; archive / unarchive / moveToDraft advertise a stronger hint than they warrant.
WIDGET_ORIGIN = _resolve_widget_origin() at import time fails closed (correct), but a misconfigured origin surfaces as a raw ValueError traceback at MCP startup rather than an operator-readable message.
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
Approve with nits. Re-reviewed at head 8b9a5465. Every substantive prior finding (model-visible backend-detail leaks, client-lifecycle and lease-release races, cancellation-safety, mutation route-key validation, backend codes out of diagnostics and telemetry) is resolved in source; only doc-level nits remain.
Still open at head 8b9a5465:
Nit, telemetry.py:8: the module docstring says the event carries "exactly four things" but then lists five bullets (adds error_category). Update the count to five or drop the word "four".
Already tracked (not re-raising): the two nits Apurva Banka (@apurvabanka) noted still stand at head, the transition_bulletin_DESTRUCTIVE_ANNOTATIONS applied to all four transitions (server.py:1314) and the import-time WIDGET_ORIGINValueError traceback (server.py:229). The #273 registration gap on server.py:288 is likewise already answered as owned by the next stacked slice.
Cross-PR: this provider is unreachable until #273 registers it in .vscode/mcp.defaults.json, so #273 should gate any rollout.
Note: the current head commit 8b9a5465 ("harden client lifecycle and telemetry") post-dates the last human and bot review comments. It resolves the outstanding cancellation-safety, discovery-leak, and discovery-telemetry items, but has not itself received a human sign-off.
Not verified: I did not run the test suites locally (no local execution) and did not exercise live Graph, Weve, or MCP-host integration. The PR-reported offline results and the host-envelope contract remain unverified by me.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
jiuqlo
left a comment
Dawn Jeong (daeunJe0ng)
left a comment
Description
Expose the separate announcements MCP provider, widget resource, manager/editor opener, audience lookup, and widget-owned authoring operations.
Current diff
titleId; create requires explicit create intent without an existingbulletinId; edit requiresbulletinId. Reject malformed combinations before generating widget retry state.config: null, retaining supplied content and localized copy titles. Opening never saves, publishes, transitions, or duplicates.isError: truewith operation-specific structured failure payloads.CommittedRefreshFailedremains non-retryable and retains the actual refresh cause without fabricating a saved ID, item, or manager result.*.devtunnels.msorigins require the explicitVORPAL_WIDGET_ALLOW_DEVELOPMENT_ORIGIN=1development gate; arbitrary HTTPS and misleading suffix hosts remain rejected.Feature contract
The opener and discovery tools are read-only. The widget owns mutations after opening. Editable copies use the create opener; their first explicit Save establishes identity. The app-only immediate-write
duplicate_bulletintool and persisted Unarchive transition remain unchanged. Unknown ownership and malformed retry requests are not accepted. Opening content for repair does not make its supplied actions valid to save or publish.The paired Vorpal consumer catches
McpToolErrorand returnserror.result, preserving the structured MCP result forisError: trueresponses. This keeps the non-retryableCommittedRefreshFailedguidance available to the widget instead of reducing it to a generic thrown error.Stack and dependency
This is slice 4/5. Head:
users/rebova/org-announcements-review-runtime. Base:users/rebova/org-announcements-prereleaseafter #271 merged on September 25, 2026. The future release target remains TBD and its final promotion baseline must be confirmed separately.Bootstrap prerequisite #262 merged into
release/planner-landing-pageon September 10, 2026. This stack remains pinned tocacb1bec056428809f1ccb0383561190d516bee4, which is an ancestor of merge commit8a04f5f40f334e729a3497877edca655730f1be2. Unchanged prerequisite work is excluded from this slice.Testing
git diff --check: passed.Readiness
Paired hosted-widget and real-backend integration remain required before rollout. This PR performs no deployment and is not release approval.