[2/5] Add agent-scoped announcement models and authoring client - #270
rebova-microsoft wants to merge 2 commits into
Conversation
rebova-microsoft
left a comment
rebova-microsoft
left a comment
There was a problem hiding this comment.
File-by-file walkthrough of the announcement API client, draft models, and supporting tests. This is an explanatory review only, not code approval or resolution of existing feedback. This PR adds client-side building blocks, not backend endpoints or the MCP announcement runtime.
| node extension.test.js | ||
| npm run validate | ||
|
|
||
| org-announcements: |
There was a problem hiding this comment.
Walkthrough — Give the new client and models their own automated checks.
This adds an Org Announcements job that sets up Python 3.11, installs the development and feature dependencies, and selects the authoring-client and draft-model tests. Keeping those two files explicit lets this layer be checked before the later directory and MCP runtime pieces are added. It describes what CI will run; it is not a deployment step or a statement that the tests have passed.
| # both of which are ignored by their own directories' rules. This entry is | ||
| # belt-and-braces only: if anything ever drops local state here it must not be | ||
| # committed. | ||
| .local/ |
There was a problem hiding this comment.
Walkthrough — Keep generated and local state out of source control.
The Python entries ignore compiled files, while .local/ is a precaution against accidentally adding local state under this feature folder. The accompanying note explains that this feature should reuse the existing shared authentication caches rather than create another private cache here. These rules only control what Git ignores; they do not create, move, or secure a token cache.
| return end is not None and end < now | ||
|
|
||
|
|
||
| class OrgAnnouncementsClient(AgentDiscoveryClient): |
There was a problem hiding this comment.
Walkthrough — Keep announcement requests tied to the chosen agent.
This is an API client, not an editor UI or backend endpoint implementation.
OrgAnnouncementsClientreuses shared authentication and agent discovery. The tenant comes from the authenticated token; the caller supplies the selected deployed ESS agent's opaquetitleId(not necessarily a GUID, and not a role)._collection_pathputs both into the intended v1.1EmployeeAgents(...)/essbulletinsroute, with no tenant-only fallback.list_bulletinsandget_bulletinread saved configurations._require_configrequires the response itself to echo the exact top-leveltitleId, rejecting missing or mismatched scope rather than filling it in.save_bulletinsends native lower-camel JSON and unwraps{id, config, errors}. HTTP 200 with errors is still a validation failure. Returned IDs are checked, not invented; an ID-bearing request updates, while no ID means create. An uncertain network/gateway failure on create is not automatically replayed: the caller must refresh before retrying to avoid duplicates.transition_bulletinsends only{id, status}, leaving preservation of stored content to the service contract.build_manager_stateprepares display data: exclude deleted rows, classify by status/schedule, preserve order, and flag a full 50-item archive window without claiming an archive total.
These client-side checks do not establish that the service routes are deployed. MCP announcement runtime wiring is separate.
| return self | ||
|
|
||
|
|
||
| class SuggestedBulletinDraft(StrictModel): |
There was a problem hiding this comment.
Walkthrough — Separate a proposed draft from saved announcement data.
This file defines data models and conversion helpers, not UI controls or backend storage.
SuggestedBulletinDraftis a partial proposal for a new announcement. It cannot supply an announcement ID, saved status, audit fields, or agent identity. Unknown fields are rejected. Suggested actions must have usable targets; ordinary draft actions may remain incomplete so publish-time completeness stays a backend concern.build_create_draftoverlays that proposal onto empty-editor defaults without adding an ID. It mapspriorityandsecondaryActionto the editor's Standard-specific field names. Suggested dates become UTC instants: a date-only start begins that UTC day and a date-only end includes its final millisecond. Ambiguous timezone-free date-times are rejected, while the original proposal is separately retained for retries.build_editor_draft_from_configinstead starts from saved content, requires its existing ID, and preserves stored schedule strings. Defaults fill missing editor values; they do not rewrite the stored record.OpenAnnouncementsRequestrequires explicitcreateoreditintent for the editor, with an ID required only for edit and suggestions limited to create. There is no republish opener mode.SaveBulletinRequestrepresents the full authored content and audience, excluding tenant/agent scope from the HTTP body. Blank dates become absent when serialized withexclude_none=True, rather than invalid date strings.
These contracts prepare data for later runtime wiring; constructing a draft does not save or publish it.
| httpx>=0.27.0,<1.0 | ||
| msal>=1.35.0 | ||
| pydantic>=2.0,<3.0 | ||
| -r ../agentconfig_core/requirements.txt |
There was a problem hiding this comment.
Walkthrough — Declare feature dependencies and reuse the shared foundation.
This dependency list includes HTTP requests, authentication, typed model validation, and the MCP library for the feature's eventual runtime. The final include pulls in agentconfig_core requirements, keeping shared authentication/cache support defined in one place instead of duplicating it here. Installing these packages supplies libraries; it does not itself register announcement tools, start a server, or make backend routes available.
| ) | ||
|
|
||
|
|
||
| def load_org_announcements_client_modules() -> dict[str, ModuleType]: |
There was a problem hiding this comment.
Walkthrough — Load just the announcement pieces these tests need.
The new helper asks the existing isolated importer for only client and drafts. Several feature folders use those same short module names, so the importer loads explicit paths and keeps feature-specific aliases to prevent one feature's tests from accidentally using another feature's client. Limiting this helper to two modules also avoids requiring the directory client, telemetry, or MCP server before those later pieces exist.
|
|
||
|
|
||
| @pytest.mark.parametrize("launch_folder", ["agentconfig_landing_page"]) | ||
| @pytest.mark.parametrize("launch_folder", ["agentconfig_landing_page", "agentconfig_org_announcements"]) |
There was a problem hiding this comment.
Walkthrough — Check shared configuration discovery from the new folder.
The existing launch-directory test now also changes into the Org Announcements folder and checks that shared configuration discovery still finds the configured environment. Tenant discovery is replaced with a test function, so this case examines path handling rather than contacting a tenant. The second change checks that the feature's requirements include the shared core. Together, they guard against needing a separate configuration or dependency setup just because a feature launches from a different directory.
| return envelope | ||
|
|
||
|
|
||
| def test_uses_agent_qualified_v11_routes_with_tenant_from_token(monkeypatch) -> None: |
There was a problem hiding this comment.
Walkthrough — Exercise the API contract without a live announcement service.
These tests use fabricated token claims and an HTTP mock to inspect routes, request bodies, and response handling. They cover token-derived tenant plus explicit agent selection, exact titleId echoes, and no tenant-only fallback. Save cases distinguish field errors inside HTTP 200, malformed responses, mismatched IDs, uncertain creates that must not replay, and updates that can retry. Manager-state cases check schedule/status classification, deleted-row exclusion, ordering, and archive-window counts. This is contract coverage, not evidence that backend routes are deployed.
| {"standardSecondaryAction": None}, | ||
| ], | ||
| ) | ||
| def test_suggested_drafts_reject_canonical_and_unknown_fields(forbidden) -> None: |
There was a problem hiding this comment.
Walkthrough — Protect the boundary between suggestions, editor state, and saves.
These model tests check that a suggestion cannot smuggle in saved identity, status, audit fields, or agent scope. They also exercise empty-editor defaults, Standard/Alert differences, action validation, original retry values, and suggested-date normalization. Separate edit cases check that existing identity and schedule strings survive conversion. Save cases check that blank dates disappear from outgoing JSON while real instants remain unchanged. The purpose is to keep helpful prefilling from silently changing stored meaning; no UI interaction or backend save is performed here.
|
Commit ace1f42 preserves repairable read-only copy content while keeping structural and service-owned write validation intact. It also exposes the Standard priority label mapping in the model-facing schema. The updated contracts are merged forward through the existing stack without rewriting history. |
Add typed announcement drafts and the tenant-and-agent-scoped authoring client. Preserve canonical save results, ownership checks, lifecycle semantics, and indeterminate-write recovery. Keep contract tests independent of the future MCP runtime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eddd3818-bb74-42d3-bcf3-7e0670a57f27
Retain typed primary actions and incomplete targets in editable copies without relaxing structural, scope, or write constraints. Explain Standard priority labels in the model-facing schema and distinguish parsing from backend action validity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15e9d51c-c328-48e6-9948-8819f9e57f90
ace1f42 to
0c8a658
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Bulletin route validation and wrapper-only save identity handling contain correctness issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds typed Org Announcements contracts and a tenant-and-agent-scoped authoring client for the staged feature stack.
Changes:
- Adds strict draft, action, schedule, and save-request models.
- Adds scoped bulletin retrieval, mutation, validation, and manager-state handling.
- Adds comprehensive contract tests and dedicated Python 3.11 CI coverage.
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds the Org Announcements test job. |
solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/.gitignore |
Excludes generated and local state. |
solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/client.py |
Implements the scoped authoring client. |
solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/drafts.py |
Defines draft and mutation contracts. |
solutions/ess-maker-skills/src/mcp/agentconfig_org_announcements/requirements.txt |
Declares feature dependencies. |
tests/mcp/_mcp_modules.py |
Adds isolated client/model loading. |
tests/mcp/agentconfig_core/test_tenant_context.py |
Extends shared-foundation assertions. |
tests/mcp/agentconfig_org_announcements/test_authoring_client.py |
Covers client behavior and scoping. |
tests/mcp/agentconfig_org_announcements/test_drafts.py |
Covers draft and save contracts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if "/" in bulletin_id or "\\" in bulletin_id or "?" in bulletin_id: | ||
| raise ValueError("bulletinId must not contain path or query separators") |
| "Org Announcements API returned a different announcement " | ||
| "than the one that was updated" | ||
| ) | ||
| return config |
Review boundary: slice 2/5, staged integration
This PR targets
users/rebova/org-announcements-prerelease, notmainor a release branch. #269 merged into that staging branch on September 18, 2026, and its shared foundation and review fixes are already included in this branch. The diff here is the announcement contracts/client slice, not a resubmission of the foundation.#271, #272, and #273 remain stacked above this PR. Reviewed slices can accumulate in prerelease, but promotion waits for the complete feature and required integration validation. Merging this slice does not deploy or approve release of Org Announcements.
Description
Add typed announcement inputs/editor drafts and the tenant-and-agent-scoped authoring client. Keep the API boundary explicit so callers cannot silently turn an update into a create or accept a record owned by another agent.
Current diff
Feature contract
Creates omit the bulletin identifier; updates supply an existing identifier. Canonical ownership remains on the configuration wrapper. Opening repairable content is read-only, not permission to persist invalid actions. API services remain responsible for authorization and action validation.
Stack and dependency
This is slice 2/5. Head:
users/rebova/org-announcements-review-contracts. Base:users/rebova/org-announcements-prerelease.The current head
0c8a65829a77ab79215506085893ba1e26895884contains the merged #269 foundation at2898c62824c9cc2649934aa1aad56940e3ab94ed. Only the two announcement-contract commits are above that staging baseline. #271 continues to target this PR's head branch, with #272 and #273 following it.Bootstrap prerequisite #262 is already inherited through the shared foundation. The future official release target and final promotion baseline remain separate decisions.
Validation
The offline contracts coverage includes repairable action preservation, priority metadata, structural rejection, authoring responses, and existing client behavior. No Graph or announcement MCP runtime modules are required by this slice.
Python 3.11 CI run 35401118877 completed successfully for the current head. It includes the dedicated Org Announcements configuration job alongside the inherited foundation and landing-page coverage. The PR Checks tab shows the latest outcome, including any additional run triggered by a description update.
Local execution previously used Python 3.13.15. No local Python 3.11 or live-service acceptance is claimed.
Review readiness for this slice
dcowen91.Separate release-promotion gates
This remains staged integration, not release approval, and performs no deployment.