[6/6] Migrate announcement authoring to the OData contract

[SophieS0ng]

Summary

• migrate Org Announcements authoring to the WeveNova EssBulletins OData routes
• adapt PascalCase OData resources and { Id, Errors } save receipts to the existing lower-camel MCP/widget contract
• validate and canonicalize GUID identities at the HTTP boundary, including verifying keyed reload responses match the requested bulletin ID
• reload canonical resources after successful saves while treating committed refresh failures as non-retryable
• preserve delete tombstone behavior without attempting an unreadable keyed reload

Stack

This is the sixth and final follow-up in the Org Announcements prerelease stack. It is based directly on the head branch of #273, so only the OData migration appears in this PR.

Testing

• 745 passed — integrated Org Announcements, setup-router, import-isolation, MCP config, and VSIX suites
• 279 passed, 1 skipped — shared AgentConfiguration core and auth suites; the skip is the expected Windows POSIX permission-bit case
• 44 passed — Maker Profile extension tests
• Ruff, production compile, and git diff --check passed

[Dawn Jeong (daeunJe0ng)]

[copilot-review] Verdict: Approve with nits.

Clean, well-tested boundary migration to the WeveNova EssBulletins OData contract. All PascalCase to lower-camel adaptation is isolated in client.py, the MCP/widget contract is untouched, GUID validation/canonicalization is added at the boundary, and the receipt + keyed-reload model correctly treats a post-commit reload failure as a non-retryable CommittedRefreshFailed (the create path is unkeyed, so replay would duplicate). No blocking issues.

Two open questions confirm backend-contract assumptions (inline), plus one nit. Strong test coverage: reload-failure parametrized across save and transition, delete-tombstone skips reload, invalid-receipt matrix, empty-GUID rejection, PascalCase error normalization. Tests bind to the public validate_* API.

Verification: reviewed the full diff of all 6 files plus the head server.py committed-refresh handling and except ordering. Did not run pytest (stacked branch on a non-default base, not checked out locally); relied on the diff and the author's stated 663 passed / Ruff-clean.

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical keyed-reload identity mismatch remains unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Validate keyed reload matches the requested bulletin ID · New

What changed in this PR

Migrates Org Announcements authoring to the EssBulletins OData contract while preserving the existing MCP/widget interface.

Changes:

• Added OData mappings, GUID validation, canonical reloads, and tombstone handling.
• Updated server, draft, client, and protocol tests.
• A critical keyed-reload identity check remains unresolved.

File	Description
tests/​mcp/​agentconfig_org_announcements/​test_mcp_app_protocol.py	Updates protocol fixtures and behavior tests.
tests/​mcp/​agentconfig_org_announcements/​test_drafts.py	Updates draft identity fixtures.
tests/​mcp/​agentconfig_org_announcements/​test_authoring_client.py	Tests OData requests, receipts, validation, and reloads.
solutions/​ess-maker-skills/​src/​mcp/​agentconfig_org_announcements/​server.py	Handles canonical refresh failures and tombstone deletes.
solutions/​ess-maker-skills/​src/​mcp/​agentconfig_org_announcements/​drafts.py	Reads canonical bulletin IDs.
solutions/​ess-maker-skills/​src/​mcp/​agentconfig_org_announcements/​client.py	Implements OData routes, mappings, validation, and reload behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Existing telemetry tests use non-GUID IDs rejected by the new validation boundary.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Update telemetry tests to use GUID IDs · New

Resolved since last review (1)

• Validate keyed reload matches the requested bulletin ID

[Copilot]

Copilot review overview

🔵 Needs a closer look

The broad contract migration retains unresolved wire-coverage gaps and warrants final human review.

Review effort: Lite
Findings: 1

Open (1)

• Update fixture to use top-level bulletin ID · New

Resolved since last review (1)

• Update telemetry tests to use GUID IDs

[Copilot]

Copilot review overview

🔵 Needs a closer look

The migration spans API contracts, lifecycle behavior, identity handling, and post-save reloads; final human review is warranted.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Update fixture to use top-level bulletin ID

[SophieS0ng]

@microsoft-github-policy-service agree company="Microsoft"

[SophieS0ng]

SophieS0ng please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

[Apurva Banka (apurvabanka)]

All 671 tests pass. Here's my review of #346.

Blocking / high

1. Enum values were not migrated with the property names — status classification is case-sensitive lowercase.

client.py maps every property name to PascalCase (Status, Bulletin, …) but leaves the values untouched: it sends "Status": "draft" and reads back with config.get("status") == "deleted" / == "retired" / == "published" (is_deleted_item, is_archived_item). A .NET OData enum normally serializes as "Draft"/"Retired"/"Deleted". If WeveNova emits PascalCase members, there is no error — deleted rows show up in the manager, retired rows count as current, and archive counts are wrong. _require_config also accepts any non-empty string as Status rather than validating it against the known member set, so nothing catches the drift. Either map/normalize status values at the boundary or assert the member set.

2. _from_odata_bulletin hard-fails the whole response if Bulletin contains Id.

Pre-migration, identity lived at config["bulletin"]["id"]. The new code raises AgentConfigApiError on Id/id inside Bulletin, and _unwrap_collection maps over items with no per-item tolerance — so one such item fails the entire ManagementView() list, i.e. a blank manager. If the OData EssBulletin complex type still carries its own Id, this is a total outage on the read path. Ignoring a redundant nested id is much safer than poisoning the collection.

3. Same fail-the-whole-list strictness on Audience.

_require_config requires Audience to be a list[str]; absent/null (a plausible "all company" encoding, and a plausible omission on ManagementView() projections) raises. Previously audience was not validated at all. Consider coercing None → [], or at minimum confirm the backend always materializes it.

Medium

4. Read mapping is now a whitelist — new/unknown backend bulletin fields are silently dropped. Combined with "the widget always sends the whole authored state", a field the backend adds is lost on read and then wiped on the next save. Previously bulletin content was passed through verbatim. Worth an explicit passthrough for unmapped keys, or a note that the contract is intentionally closed.

5. Read-after-write lag is reported as an unretryable committed failure. Every write now does Save + a keyed GET. On the unkeyed create path, a replica lag 404 surfaces as CommittedRefreshFailed ("do not repeat the action") for what is the most common operation. A short bounded retry on 404 for the post-save reload would avoid teaching makers to distrust a normal create. Also note this doubles round trips on every mutation.

6. CommittedCanonicalReloadError is a bare Exception outside _MUTATION_ERRORS. All three current call sites catch it explicitly, but a future caller of save_bulletin/transition_bulletin that forgets escapes as an unhandled FastMCP ToolError rather than a structured result. The file's own comment says _MUTATION_ERRORS exists "so an exception can't escape one step but not the other" — this new type breaks that invariant.

7. Non-GUID legacy ids are now permanently unloadable. validate_bulletin_id requires a parseable UUID. Is the backend guaranteed to have re-keyed every existing announcement before this ships? Worth stating in the PR description, since there's no fallback.

Minor / stale

• client.transition_bulletin hardcodes status == "deleted" to skip the reload, duplicating server.TRANSITION_STATUS["delete"] — silent break if that map changes.

• transition_bulletin's comment still says "The canonical changed row is included alongside the manager state"; it's now conditional on changed is not None. The widget contract change (no item on delete) isn't called out.

• _COPY_STRIPPED_FIELDS (id, createdBy, status, version, etag, …) is now dead code on the duplicate path — the read whitelist can never produce those keys.

• is_archived_item / ARCHIVED_WINDOW_SIZE docstrings still say "the list carries no envelope metadata"; responses are now an OData value envelope.

• save_bulletin's docstring still refers to "the authoring save endpoint" (now Save).

[Dawn Jeong (daeunJe0ng)]

[copilot-review]
Approve with nits. Re-reviewed at current head aba6f404. The substantive findings are already captured by Dawn Jeong (@daeunJe0ng)'s inline threads and Apurva Banka (@apurvabanka)'s review, so this comment does not repeat them; it reconciles those reviews against the three commits that landed afterward (d08a7cd, a9b78d2, aba6f404) and adds one item.

Resolved by the newer commits

• Read-after-write on the unkeyed create path (apurvabanka Medium 5): d08a7cd added a bounded, 404-only keyed-GET retry (client.py _reload_committed_bulletin, delays 0.05/0.15), so a replica-lag 404 no longer surfaces as an unretryable committed failure.

Still open at current head (I concur with the prior reviewers; not re-listing each)

• The strict read path and read whitelist (Dawn Jeong (@daeunJe0ng) client.py:491; apurvabanka Blocking 1-3 and Medium 4) are unchanged by d08a7cd. Context that reframes them: the new provenance fixture wevenova_ess_bulletin_contract.json pins the WeveNova DTO (commit fa4679cf74) to lowercase status values, contentHasId: false, and audience non-null default []. That is the source-of-truth evidence behind the casing, id-location, and audience assumptions. Residual asks: confirm the production WeveNova backend conforms to that pinned commit, and consider per-item tolerance on the list read so one drifting row cannot blank the whole ManagementView.

One item I did not see raised

• The bulletin id moved to root config.id (from config.bulletin.id). Please confirm the WeveNova/Vorpal widget reads the root path so authored rows do not render blank.

Process

• d08a7cd, a9b78d2, and aba6f404 all post-date every prior review, including the 22:39 UTC pass, so the contract-enforcement changes and the fixture have not yet had a reviewer pass.

Not verified

• No local pytest run (stacked branch on a non-default base) and no live WeveNova integration. The casing, contentHasId, and audience-nullability claims rest on the checked-in fixture, not a live response.

[Dawn Jeong (daeunJe0ng)]

Looks good to merge for this urgent OData slice. Non-blocking follow-ups are captured in my earlier [copilot-review] comment above.