fix(setup): keep local authoring independent of publishing

[nkemms]

Summary

• make workspace attachment independent of published Dev configuration for existing Dev, supplied-package, Prod-to-Dev, and MOS starter setup paths
• validate the direct Dev route and authoritative component identity/schema before materializing local files
• separate local workspace progress from FlightCheck-backed runtime readiness and render readiness in a fixed table
• replace the verbose workspace summary with a friendly product link to the exact Copilot Studio agent
• make MOS catalog choices use inferred friendly product names with the service-provided name as the fallback
• make FlightCheck freshness fixtures deterministic on Windows filesystems

Why

A newly created MOS agent can expose editable components before it has published Dev configuration. Test 31 reproduced setup incorrectly treating the missing published /configure resource as a requirement to publish. Test 33 confirmed that unpublished attachment succeeds, but also exposed that the progress checklist conflated completed local materialization with overall runtime readiness.

Runtime evidence

• unpublished MOS attachment materialized 44 topics and 36 variables through the direct Dev route and component endpoint
• capacity and connection FlightCheck findings remained visible without reverting the completed materialization stage
• the final handoff links directly to the exact Copilot Studio agent and reports all four readiness checks separately

Tests

python -m pytest tests\scripts\test_setup_existing_da.py tests\scripts\test_setup_mos_starter.py tests\scripts\test_setup_alm_import.py tests\scripts\test_setup_alm_export.py tests\scripts\test_auth.py tests\scripts\test_da_command_degradation.py tests\setup\test_da_setup_router.py -q

227 passed.

[Apurva Banka (apurvabanka)]

Gaps found

1. Native ALM API version applied inconsistently — agentbuilder.py (highest confidence)

import, export, components, publish, and update_bot_entity moved to 2022-03-01-preview, but two sibling routes on the same /copilotstudio/minimalBots/alm/ surface were left on self.api_version (2024-10-01):

• get_realms → /alm/{id}/realms (line ~807)

• get_realm_configuration → /alm/{id}/configure (line ~826)

get_realms is now on the critical attach path (require_alm_family=False always). The mock still pins configure?api-version=2024-10-01, so tests can't catch a mismatch. Either these were deliberately verified as GA-versioned (worth a comment) or they're a live-break waiting to happen.

2. Schema mismatch is detected after canonical state is written — setup_existing_da.py:1940-1996

The prefetch calls _validate_changeset_identity(prefetched_changeset, normalized_agent_id) without expected_schema_name, then assigns the fetched schema to connection["agent"]["schemaName"]. _validate_setup_target + _record_canonical_setup_progress then persist that foreign schema into .local/setup/config.json before the second validation (line 1993) rejects it. Previously the expected schema was the fallback, so state was never poisoned. Fix: pass expected_schema_name=expected_schema_name to the prefetch call.

3. almFamilyId is now permanently None on attach

require_alm_family=False is unconditional, so _confirm_dev never runs from attach and family identity is never discovered — the preservation branch at 1960-1964 can only echo pre-existing state that nothing writes anymore. The consistency check in _canonical_agent_matches_connection (589-592) becomes vacuous as a result. setup_alm_export.py re-derives it from grsRepositoryId, so this is probably intentional, but the dead preservation branch and the now-unreachable check should be acknowledged.

4. _validate_changeset_identity got stricter for all paths

It now raises "Component fetch did not return a schema name." unconditionally. Previously that requirement only applied when expected_schema_name was set (alm-import). Any existing-dev agent whose components omit bot.schemaName will now hard-fail attach where it used to succeed.

5. Stale fixtures / lost coverage (minor)

• tests/fixtures/cassettes/connectivity_connections.yaml and agentbuilder_readiness.yaml still record x-ms-client-name: EssAdk.

• import_package/export_package now build headers from scratch and silently drop Accept: application/json (previously inherited). Intentional?

• test_publish_routes_da_ga_to_native_client flipped its fixture to ValidationPending: True, so the native "Published" success branch is no longer asserted anywhere.

CC: nkemms

[nkemms]

Thanks for the detailed pass. I traced each item against the platform HTTP reference, live captures, and the current attach state model.

1. API versions — intentional endpoint-specific split. The platform almimportexport.http reference explicitly uses 2022-03-01-preview for components update/fetch, publish, export, and import. It does not cover realms or configure; configure is live-proven on 2024-10-01, and the setup transcript also exercised realm discovery successfully at the configured default version. I am leaving those read routes on DEFAULT_API_VERSION rather than moving a live-proven route based only on the shared path prefix.

2. Schema mismatch before canonical write — not reachable. When expected_schema_name is supplied, _confirm_dev_route uses it as the fallback schema, so the schema-prefetch branch cannot run. That branch is reached only when the expected schema is empty; passing it into the first validation would therefore be inert. A supplied schema that conflicts with a card schema is rejected earlier.

3. almFamilyId — intentional for publish-independent attachment. Attachment now proves exact ID + Dev realm + authoritative component schema without requiring published Dev configuration. Cross-realm family proof still happens where needed (validate-agent, Prod-to-Dev matching, and guarded import), and an existing canonical family ID is preserved. Fresh attachment may persist null, but no workspace operation consumes that field. If lineage needs to be retained later, it should be carried from an already verified receipt rather than rediscovered through a publish-dependent configure call.

4. Required component schema — intentional fail-closed behavior. The live component BotEntity contains the authoritative schema even when the direct agent card does not. Materialization cannot safely name and bind the workspace without it, so a component response lacking schema should fail rather than silently continue.

5. Fixtures and coverage. The cassettes are immutable recordings of earlier live traffic and should not be hand-edited; current unit tests separately assert CopilotStudio. Omitting Accept on import/export matches the exact external request reference (export returns ZIP; import is multipart). The publish-output coverage point is valid: changing the fixture to PascalCase pending stopped exercising the lowercase published-success path. I have added local parameterized coverage for both {"ValidationPending": true} and {"validationPending": false}; that test-only change is not pushed yet.

[RamananVr]

Code Review — PR #293

Mode: Lightweight (diff-based) review, cross-checked against full PR-branch file snapshots (git show pr293:<path>).

I largely arrive at the same conclusions as Apurva Banka (@apurvabanka)'s review above, and confirmed each of their points independently against the actual source (line numbers below refer to the PR branch). Two of these are BLOCKER-level, not just observations.

[BLOCKER] 1. Canonical setup state can be persisted with an unvalidated/wrong schema name before the real validation runs

solutions/ess-maker-skills/scripts/setup_existing_da.py:1938-1997 (attach_existing_dev):

prefetched_changeset: dict[str, Any] | None = None
if not connection["agent"]["schemaName"]:
    prefetched_changeset = client.fetch_components(normalized_agent_id)
    connection["agent"]["schemaName"] = _validate_changeset_identity(
        prefetched_changeset,
        normalized_agent_id,
    )  # <-- no expected_schema_name passed here
canonical_state, existing_setup = _validate_setup_target(kit_root, connection)
...
if existing_setup is None or progress_recorded:
    _record_canonical_setup_progress(kit_root, connection, canonical_state, existing_setup)  # <-- writes .local/setup/config.json
    progress_recorded = True
...
_validate_changeset_identity(
    changeset,
    normalized_agent_id,
    expected_schema_name=expected_schema_name or schema_name,  # <-- the real check, can raise here
)

_record_canonical_setup_progress -> _store_canonical_agent_state -> _write_json(kit_root / CANONICAL_SETUP_STATE, ...) is a real disk write of schema_name into .local/setup/config.json, and it happens before the second, schema-aware _validate_changeset_identity call that can reject the schema. On rejection, _try_record_canonical_setup_blocked only flips the SETUP-07 step to "blocked" — it never clears the already-persisted schema_name. So a caller that supplies expected_schema_name (alm-import / prod-to-dev / mos-starter flows) can end up with local state poisoned by a foreign agent's schema even though the attach was correctly rejected.

Test coverage confirms this isn't just theoretical: test_receipt_backed_attach_rejects_component_schema_mismatch (tests/scripts/test_setup_existing_da.py:810-835) only asserts the workspace directory wasn't created — it never checks CANONICAL_SETUP_STATE. Reproducing the scenario shows canonical state is written to disk with a "blocked" step before the error propagates, which the test's own name implies shouldn't happen.

Fix: pass expected_schema_name=expected_schema_name into the first (prefetch) _validate_changeset_identity call so the mismatch is caught before any disk write, or defer _record_canonical_setup_progress until after the second validation succeeds.

[BLOCKER] 2. get_realms / get_realm_configuration were left on the old API version, but are now unconditionally on the critical attach path

solutions/ess-maker-skills/scripts/agentbuilder.py:804-832:

def get_realms(self, agent_id: str) -> dict[str, Any]:
    body = self._json(
        "GET",
        f"/copilotstudio/minimalBots/alm/{agent_id}/realms",
        "Agent realm family",
    )  # no params override -> defaults to self.api_version (DEFAULT_API_VERSION = "2024-10-01")

def get_realm_configuration(self, agent_id: str, realm: int) -> dict[str, Any]:
    ...
    body = self._json(
        "GET",
        f"/copilotstudio/minimalBots/alm/{agent_id}/configure",
        operation,
        params={"realm": realm},
    )  # same — still 2024-10-01

Every sibling route on the same /copilotstudio/minimalBots/alm/ surface touched by this PR (import, export, components, publish, update_bot_entity) was deliberately moved to NATIVE_ALM_API_VERSION = "2022-03-01-preview". These two were not. That would be a minor inconsistency on its own, but require_alm_family is now hardcoded to False in attach_existing_dev (was previously setup_source != "alm-import"), which means validate_existing_dev_connection's else branch — and therefore client.get_realms(...) (setup_existing_da.py:1249) — now runs on every attach, for every setup source, not just as an edge case.

If the native ALM surface actually requires 2022-03-01-preview (as implied by every other route's migration), this is either a live bug waiting to happen or was deliberately verified as fine on the GA version — but nothing in the PR says which, and no test would catch a mismatch: the mock fixture (tests/mocks/agentbuilder_connectivity.py:120) still pins /configure?api-version=2024-10-01, and there's no test asserting api-version specifically for the /realms GET (only a URL-suffix check).

Ask: please confirm explicitly (ideally with a code comment) whether get_realms/get_realm_configuration are intentionally staying on DEFAULT_API_VERSION, or whether they were missed in the migration and should move to NATIVE_ALM_API_VERSION like their siblings.

[Non-blocking] 3. Native "Published" success branch in _publish_native is no longer exercised by any test

test_publish_routes_da_ga_to_native_client (tests/scripts/test_da_command_degradation.py:38) now has FakeClient.publish_agent return {"ValidationPending": True} exclusively, so publish.py's _publish_native only ever takes the "still validating" branch in tests. The success branch (prints "Published. Pushed topic changes are now live.") isn't asserted anywhere in the suite anymore. Worth adding a parametrized case with ValidationPending: False/absent to lock in that message.

[Non-blocking] 4. almFamilyId is now permanently None on attach — please confirm this is intentional

With require_alm_family=False unconditional, _confirm_dev never runs from attach_existing_dev, so almFamilyId is never discovered via that path. The preservation branch at setup_existing_da.py:1958-1964 can now only echo pre-existing state that nothing new ever populates, and the consistency check in _canonical_agent_matches_connection (~589-592) becomes effectively vacuous for attach-originated state. setup_alm_export.py apparently re-derives family identity from grsRepositoryId separately, so this is probably intentional — but the dead preservation branch and now-unreachable check are worth a comment (or removal) so a future reader doesn't assume they're still load-bearing.

[Question] 5. import_package/export_package header rebuild drops Accept: application/json

Headers are now built from scratch ({"Authorization": ..., "x-ms-client-name": ...}) instead of filtering self.headers to exclude only Content-Type. Tests were updated to match (confirms this is intentional, not an accident), but there's no comment explaining why Accept is no longer sent on these two multipart routes specifically. A one-line rationale would help future maintainers.

Minor / FYI (not blocking)

• import_package's hardcoded "package.zip" filename (replacing package_path.name) is intentional per updated tests — fine, but a short comment on why the filename must be literal would help (e.g., if the native ALM import endpoint requires it regardless of source path).
• _validate_changeset_identity's schema-presence check is now unconditional (previously only enforced when expected_schema_name was set) — this is a real tightening of behavior for existing-dev attach, and appears intentionally covered by test_existing_dev_attach_derives_schema_from_components, but is worth calling out explicitly as a behavior change for anyone bisecting a regression later.

---

Verdict recommendation: Request changes. Items 1 and 2 are both plausible live bugs on the primary attach path (state poisoning on schema mismatch, and a possibly-wrong API version on every attach call) and should be resolved or explicitly justified before merge. Items 3-5 are good to address but not blocking.

[Apurva Banka (apurvabanka)]

Thanks for the thorough replies — I went back and verified each one against the code rather than just the diff.

Retracting my #2 (and the matching BLOCKER in the review below). You're right that it isn't reachable. I instrumented _validate_changeset_identity to confirm: when expected_schema_name is supplied, _confirm_dev_route uses it as the fallback, so schemaName is never empty and the prefetch branch never runs — the mismatch is caught on the first and only validation. When it isn't supplied, the prefetch runs but the second call resolves to the schema it just derived, so there's nothing to disagree with. The fix I proposed would have been a no-op.

I also confirmed the persisted schema_name on a rejected attach is the expected schema with a blocked step, not a foreign one — so no state poisoning. I saw the write happening before validation and wrongly inferred what was in it. Apologies for the noise.

Your answers on #3 (family identity moved to validate-agent / prod-to-dev / guarded import), #4 (fail-closed on authoritative component schema), and #5 (cassettes are immutable recordings; Accept omission matches the external request reference) all check out. Publish coverage is now in via 8db835fe — verified passing locally.

One optional nit: a one-line comment on get_realms / get_realm_configuration noting they intentionally stay on DEFAULT_API_VERSION because the almimportexport.http reference doesn't cover them and configure is live-proven on 2024-10-01. Two of us asked the same question independently, so the next reader probably will too. Not worth holding the merge for.

Approving. 🚀

[nkemms]

Apurva Banka (@apurvabanka) I'll align the API versions to the newer value in a follow up PR. Thanks

[nkemms]

Thanks for the independent pass, RamananVr. These substantially overlap with Apurva Banka (@apurvabanka)'s earlier review; after your comment, Apurva instrumented the schema path and retracted that blocker—the persisted value is the expected schema, not a foreign one, so the proposed prefetch change would be inert.

The API-version split will be aligned to 2024-10-01 in a follow-up PR. Publish-success coverage landed in 8db835fe. The absent fresh-attach almFamilyId, fail-closed component schema requirement, and multipart/ZIP headers are intentional and were accepted in Apurva's follow-up approval. I'll also carry the package.zip rationale as a small documentation improvement in the follow-up. Thanks again.