[1/5] Share authoring identity and deployed-agent discovery - #269
Conversation
Extract read-only deployed-agent discovery and reuse tenant-local account matching. Preserve account-bound token refresh and safe authentication failures. Add isolated regression coverage and narrowly scoped review-stack CI targets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eddd3818-bb74-42d3-bcf3-7e0670a57f27
|
Review order: #269 (shared core/discovery) → #270 (contracts/client) → #271 (Graph audience) → #272 (MCP runtime) → #273 (maker/setup/packaging). Each PR targets the preceding review branch; #269 targets Bootstrap PR #262 has merged, and the pinned base is in the release branch ancestry. Promotion to the future official release branch and |
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
🟡 Changes recommended
Unreadable token files can still expose private filesystem paths, and the test importer suppresses unrelated warnings.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Extracts reusable deployed-agent discovery and authoring identity handling while preserving landing-page behavior and enabling stacked-review CI targets.
Changes:
- Adds shared discovery, identity validation, and privacy-focused credential refresh handling.
- Adds isolated MCP imports and regression coverage.
- Expands CI pull-request targets for the announcement review stack.
File summaries
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds review-stack targets and auth tests. |
agentconfig_core/_odata.py |
Centralizes title ID validation. |
agentconfig_core/agent_discovery.py |
Adds shared read-only discovery client. |
agentconfig_core/base_client.py |
Adds identity matching and sanitized refresh errors. |
agentconfig_landing_page/client.py |
Delegates discovery to the shared client. |
tests/mcp/_mcp_modules.py |
Adds collision-free MCP test imports. |
tests/mcp/conftest.py |
Isolates tests from real credentials and networks. |
tests/mcp/agentconfig/test_client.py |
Uses the isolated importer. |
tests/mcp/agentconfig/test_server_contract.py |
Uses the isolated importer. |
tests/mcp/agentconfig/test_widget_protocol.py |
Uses the isolated importer. |
tests/mcp/agentconfig_core/test_agent_discovery.py |
Covers shared discovery behavior. |
tests/mcp/agentconfig_core/test_base_client.py |
Expands identity and refresh coverage. |
tests/mcp/agentconfig_core/test_ci_contract.py |
Verifies CI trigger policy. |
tests/mcp/agentconfig_core/test_tenant_context.py |
Covers tenant discovery and dependencies. |
tests/mcp/agentconfig_core/test_token_cache.py |
Verifies private lock-file permissions. |
tests/mcp/agentconfig_core/test_token_refresh.py |
Verifies sanitized refresh failures. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
rebova-microsoft
left a comment
rebova-microsoft
left a comment
There was a problem hiding this comment.
File-by-file walkthrough
These 16 explanatory comments describe what each changed file does and why it belongs in this shared-foundation slice. They are reading aids, not an approval or a request for changes, and do not resolve the existing review feedback.
Scope: 4 runtime files, 1 CI file, and 11 test/support files. Of the 929 added lines, 774 are under tests/. This PR extracts shared discovery and identity primitives; it does not yet add the announcement provider or widget.
Suggested reading order: agent_discovery.py alongside the landing-page client deletions, then base_client.py with its regression cases, then the test-import support and CI changes.
| return _validate_odata_string(value, name).replace("'", "''") | ||
|
|
||
|
|
||
| def _validate_title_id(title_id: str) -> str: |
There was a problem hiding this comment.
Walkthrough — shared titleId validation.
This moves the existing landing-page title-ID rule into the shared helpers: reuse the established string validation and reject identifiers longer than 256 characters. titleId stays an opaque deployed-agent identifier, not a GUID requirement.
The purpose is one rule for both feature clients. This validator does not replace OData key escaping; callers still pass validated IDs through the existing encoding helper when constructing an agent URL.
| ) | ||
|
|
||
|
|
||
| class AgentDiscoveryClient(AgentConfigBaseClient): |
There was a problem hiding this comment.
Walkthrough — extract discovery once, without adding a server.
This new Python client contains the agent-list/search behavior moved out of the landing-page client, along with the existing payload-casing helpers and collection unwrapping. It inherits authentication, HTTP transport, and retries from AgentConfigBaseClient.
The operations remain GET tenants('{tenantId}')/EmployeeAgents and POST .../SearchAgents. Search trims its input and rejects empty or overlong strings. Responses may be a bare list or {value: [...]}; an unexpected shape raises an error rather than becoming an empty success.
The separate _agent_collection_path and explicit discovery conversion keep lookup independent of a feature's own collection route and canonical payload format. Landing-page consumes this shared code in this slice; announcements consumes it later. No third MCP process or configuration-initialization write is introduced.
|
|
||
|
|
||
| class AgentConfigClient(AgentConfigBaseClient): | ||
| class AgentConfigClient(AgentDiscoveryClient): |
There was a problem hiding this comment.
Walkthrough — the deleted discovery code moved to the shared layer.
AgentConfigClient now inherits from AgentDiscoveryClient, which itself inherits from AgentConfigBaseClient. This removes local copies of list/search, key-casing helpers, and title-ID validation; it does not remove those capabilities.
Landing-page-specific create/get/update operations stay here. The client retains its base URL and normal payload conversion, and _collection_path() delegates to the shared agent-collection path.
Read this deletion alongside agentconfig_core/agent_discovery.py: the intent is to preserve existing landing-page behavior while allowing a second feature provider to reuse discovery without calling the landing-page MCP process.
|
|
||
|
|
||
| def _refresh_msal_token(tenant_id: str, object_id: str) -> str: | ||
| def account_for_identity(app: Any, tenant_id: str, object_id: str) -> Any | None: |
There was a problem hiding this comment.
Walkthrough — reusable identity checks and controlled credential failures.
This is the main behavioral file in the slice; it is not a new login/token-cache implementation.
account_for_identityextracts the tenant-profile matching already used by token refresh. MSAL groups profiles by home account, so matching the tenant-local cached profile matters for identities such as guest accounts. The helper returns an account only when the match is unambiguous.object_idexposes the account captured by the authoring client.validate_token_identitycentralizes the tenant/account comparison so later resource clients can reuse it. Backend token validation and authorization remain authoritative.- Decoded token payloads must be JSON objects; malformed payloads are rejected explicitly.
- Missing/empty token-file messages and MSAL failure messages stop echoing private paths or raw provider details. During refresh, validation, cache-lock, and filesystem failures become a controlled 401
AgentConfigApiError, retaining the original exception as its cause.
The account-preserving refresh mechanism was already upstream. This slice extracts its matching/checking logic and changes error handling. The separate existing review thread about an unreadable token file during initial construction remains open; this walkthrough does not claim that path is fixed.
| branches: | ||
| - "main" | ||
| - "release/**" | ||
| - "users/rebova/org-announcements-prerelease" |
There was a problem hiding this comment.
Walkthrough — run CI for this review stack, without opening every feature branch.
The PR-target filter adds exactly our prerelease branch and users/rebova/org-announcements-review-*. Without those entries, the stacked PRs would miss CI because they target each other rather than main or release/**.
The other change in this file adds the existing tests/scripts/test_auth.py to the foundation job because authentication is a shared dependency. Existing push filters, PR event types, and permissions are retained. The announcement-specific CI job is not part of this first slice; it arrives with the announcement modules in subsequent chunks.
| ) | ||
|
|
||
|
|
||
| def test_the_sign_in_notice_goes_to_stderr_not_stdout(monkeypatch) -> None: |
There was a problem hiding this comment.
Walkthrough — why this is the largest added test block.
The expanded cases cover shared behavior both feature clients rely on: credential-source precedence, configured-tenant matching before a request, silent refresh of the original identity, rejected/mismatched replacement credentials, concurrent 401s sharing one refresh, privacy-safe surfaced failures, and checkout-anchored imports.
The sign-in tests also ensure human-readable browser notices go to stderr rather than corrupting MCP's JSON-RPC stdout stream. Tests use fake servers, accounts, tokens, and HTTP transports.
Much of this protects behavior already in the upstream foundation; the line count does not imply that this PR adds hundreds of lines of authentication runtime. The new assertions complement the account-helper extraction and error normalization in base_client.py.
| import yaml | ||
|
|
||
|
|
||
| def test_ci_accepts_only_the_approved_review_targets_in_addition_to_upstream(): |
There was a problem hiding this comment.
Walkthrough — make the narrowly approved CI boundary explicit.
This test asserts the exact push and PR-target branch lists, the existing PR event types, and contents: read permissions. Its purpose is to catch accidental broadening of repository-wide CI policy while adding support for this particular stack.
It uses yaml.BaseLoader so the workflow key on remains a string rather than being interpreted as a YAML 1.1 boolean. These assertions are intentionally policy-specific; a deliberate future policy change should update them alongside the workflow.
|
|
||
|
|
||
| @pytest.mark.parametrize("launch_folder", ["agentconfig_landing_page"]) | ||
| def test_discovers_configured_environment_from_feature_launch_directory( |
There was a problem hiding this comment.
Walkthrough — tenant discovery must work from a feature's launch directory.
This extends coverage for running from the landing-page MCP directory, parsing mocked Dataverse authentication challenges into a concrete tenant, and ensuring runtime installation paths include the shared dependencies.
The earlier configuration-path assertion is adjusted because the new automatic fixture deliberately redirects _CONFIG_PATH into a temporary location. The test still verifies the solution root and imported auth.py belong to this checkout, not a sibling worktree.
Only the landing-page launch directory is exercised in this first slice; future announcement files are not required. The tenant-discovery implementation and dependency declarations already exist in the upstream baseline and are not rewritten here.
|
|
||
| assert stat.S_IMODE(directory.stat().st_mode) == 0o700 | ||
| assert stat.S_IMODE(path.stat().st_mode) == 0o600 | ||
| assert stat.S_IMODE(Path(str(path) + ".lockfile").stat().st_mode) == 0o600 |
There was a problem hiding this comment.
Walkthrough — one additional permissions assertion.
The existing POSIX permissions test already checks the cache directory (0700) and cache file (0600). This line extends the check to the synchronization lock file, also requiring 0600.
The token-cache implementation is unchanged in this PR. This is a small extension of the existing private-file coverage, not a new cache design.
| async def run(): | ||
| try: | ||
| with pytest.raises(ValueError, match="does not exist"): | ||
| with pytest.raises(base_client.AgentConfigApiError) as caught: |
There was a problem hiding this comment.
Walkthrough — match the controlled refresh-error contract.
When the original token file disappears, refresh now surfaces a 401 AgentConfigApiError rather than a raw ValueError. The updated assertion checks the HTTP status, retains the underlying ValueError as the cause, and confirms the private file path is absent from the surfaced message.
The existing scenario still guards against silently switching from that original file to a different credential source. This test change follows the normalization in _refresh_token; it does not relax identity preservation or assert that initial token-file construction errors use the same path.
Normalize token-file I/O failures without exposing the configured path or falling back to another credential source. Preserve the original exception as the cause. Limit MCP test-loader warning suppression to the known Pydantic category and cover caller warning policies and module cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eddd3818-bb74-42d3-bcf3-7e0670a57f27
| branches: | ||
| - "main" | ||
| - "release/**" | ||
| - "users/rebova/org-announcements-prerelease" |
There was a problem hiding this comment.
I don't think we want to add user branches for CI.
There was a problem hiding this comment.
Agreed that these user-branch entries should not ship in the final release. They are temporary support for reviewing this stack: #269 targets users/rebova/org-announcements-prerelease, and the downstream PRs target the preceding review branch rather than main or release/**.
The pull_request.branches filter matches the PR target branch, so the existing filters alone would skip CI for these PRs. The push trigger is unchanged and still restricted to main and release/**.
I also confirmed this is working: CI run 34908684999 ran on September 14 as a pull_request event for #269, associated with the current head c1afef5c and targeting the prerelease branch. All five CI jobs executed and passed, including their actual lint/test steps; these are not inherited checks from another branch.
I will remove both temporary user-branch filters before promoting the changes to the final release branch. Once the PRs target a release/** branch, the existing release filter covers them.
|
Blocking
Design / layering
Diagnostics
Scope
Minor: CC: rebova-microsoft |
|
Thanks Apurva Banka (@apurvabanka). I have updated the description to make the staging boundary and cleanup plan much more explicit. #269 is slice 1 of a five-PR stack, merging only into Specific pointers for the forward-reference/sharing questions:
CI cleanup: agreed that changing only the workflow would leave a failing assertion. Before final release promotion, I will remove both temporary user-branch target filters and update Dependencies/layering: the foundation job installs both Diagnostics and remaining observations: the JWT decode, token-file read exception, and normalized refresh paths retain chained causes, while their returned messages are sanitized. The MSAL failure message is coarser, so I am not claiming all diagnostic detail is preserved. The diagnostics tradeoff, loader import identities/partial-load caching, and remaining minor observations still merit individual triage; the staging explanation does not resolve them. I am not proposing to restore raw authentication/provider payloads or treating those concerns as closed. The description also now explains why the shared-auth boundary changes and isolation/regression support are included alongside the extraction. This update is description/discussion only; no implementation changes or review resolutions are being made. |
Keep known credential failures actionable through MCP responses without exposing private details. Distinguish local sign-in timeout and cover plain/widget error serialization while preserving account binding and replay behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eddd3818-bb74-42d3-bcf3-7e0670a57f27
|
Apurva Banka (@apurvabanka) I dug into the remaining points and found a real issue with the auth diagnostics: the underlying cause was preserved in Python, but the MCP response only showed the generic outer message. Addressed that in b559173. Known credential failures now explain what needs attention without exposing paths, tokens, or raw provider messages. The local sign-in timeout also has its own message. Account selection and retry behavior are unchanged. The importer concerns didn't reproduce as bugs: both helper imports share the same provider-module cache, and partial-load retries behave consistently. I'm leaving that code alone rather than adding a refactor to this PR. The temporary CI filters and their matching assertion are still scheduled for removal before release promotion. Ready for another look when you have a chance. |
2898c62
into
users/rebova/org-announcements-prerelease
Review boundary: slice 1/5, not a release
This PR targets
users/rebova/org-announcements-prerelease, a staging/integration branch for the complete Org Announcements feature. It does not targetmainor a release branch and performs no deployment. Reviewed slices can accumulate there, but promotion waits for the complete feature, the remaining reviews, and the required integration validation. Merging this foundation slice is not approval to ship Org Announcements.Each downstream PR targets the preceding review branch, not the prerelease branch directly. Each diff is therefore one review slice against its immediate predecessor. The existing landing-page behavior must remain intact at this stage; the staged destination does not waive correctness of the code in this slice.
Description
Extract a neutral, read-only deployed-agent discovery client and delegate the existing landing-page lookup tools to it. Expose shared tenant-local account matching and privacy-safe credential checks for the subsequent announcement slices, without adding another MCP process.
The downstream consumers are already present in the published stack: #270 adds
OrgAnnouncementsClient(AgentDiscoveryClient), #271 uses the shared identity helpers for Graph, and #272 loads both feature providers and extends the shared discovery coverage to both. The announcement loader entry point in this slice prepares that later coverage; it is not called by this slice, which does not yet contain the announcement runtime.Current diff and rationale
Feature contract
Discovery returns deployed agent identifiers without initializing feature configuration. Tenant/account context comes from authentication rather than tool arguments. Feature providers retain separate MCP processes while reusing the neutral core.
Dependency and promotion baseline
Bootstrap prerequisite #262 merged into
release/planner-landing-pageon September 10, 2026. This stack was prepared againstcacb1bec056428809f1ccb0383561190d516bee4, an ancestor of merge commit8a04f5f40f334e729a3497877edca655730f1be2; unchanged prerequisite work is excluded from this slice. The future official release target and its final promotion baseline must be confirmed separately.Validation
Local validation used Python 3.13.15; no local Python 3.11 run is claimed. Focused offline regressions cover the shared auth paths and both registered MCP response boundaries.
GitHub Actions uses Python 3.11. CI run 35395516201 tracks the auth-diagnostics follow-up at
b5591731667393d2fae78419efe08cb83db8a7ac; the PR Checks tab shows the current outcome, including any additional run triggered by a description update. The workflow covers Python lint, installer smoke tests, MCP foundation tests, landing-page configuration, and FlightCheck offline tests.This does not cover live Graph, authoring-service, hosted-widget, or end-to-end integration; those remain required before rollout.
Temporary CI support and removal plan
pull_request.branchesfilters the target branch, so the existingmain/release/**rules alone do not cover these stacked PRs. The following entries are temporary:users/rebova/org-announcements-prereleaseusers/rebova/org-announcements-review-*Before final release promotion, remove both entries and update the matching assertion in
tests/mcp/agentconfig_core/test_ci_contract.pyin the same change. Preserve the ordinary branch filters, PR event policy, and read-only workflow permissions. The existingrelease/**filter covers a release target using that naming convention; confirm coverage when the final target is chosen. Keep stack coverage in place until that transition.Review readiness for this slice
Separate release-promotion gates
This remains a staged implementation review, not release approval.