fix: address PR review comments — revert api-version default, remove MCP embedding, add ServicePolicy test

Agent-Logs-Url: https://github.com/Azure/apiops-cli/sessions/8711e436-2a22-4c8b-b435-5f7a7e17847a

Co-authored-by: EMaher <9244742+EMaher@users.noreply.github.com>

Author: Copilot

package-lock.json

@@ -75,7 +75,7 @@ async function executeExtract(
   }
 
   // Build service context
-  const apiVersion = globalOpts.apiVersion ?? process.env.AZURE_API_VERSION ?? '2024-10-01-preview';
+  const apiVersion = globalOpts.apiVersion ?? process.env.AZURE_API_VERSION ?? '2024-05-01';
   const cloudName = globalOpts.cloud ?? 'public';
   const cloudConfig = getCloudConfig(cloudName);
   const baseUrl = buildArmBaseUrl(cloudName, subscriptionId, options.resourceGroup, options.serviceName);

src/cli/extract-command.ts

@@ -83,7 +83,7 @@ async function executePublish(
 
   // Build service context
   const apiVersion =
-    globalOpts.apiVersion ?? process.env.AZURE_API_VERSION ?? '2024-10-01-preview';
+    globalOpts.apiVersion ?? process.env.AZURE_API_VERSION ?? '2024-05-01';
   const cloudName = globalOpts.cloud ?? 'public';
   const cloudConfig = getCloudConfig(cloudName);
   const baseUrl = buildArmBaseUrl(

src/cli/publish-command.ts

@@ -59,15 +59,7 @@ export async function publishApi(
 
     // Step 3: Publish child resources in parallel
     // When a spec was imported, operations and schemas are auto-created by APIM
-    await publishApiChildren(
-      client,
-      store,
-      context,
-      descriptor,
-      config,
-      rootResult.specImported,
-      rootResult.embeddedMcpApplied,
-    );
+    await publishApiChildren(client, store, context, descriptor, config, rootResult.specImported);
 
     return {
       descriptor,
@@ -111,26 +103,6 @@ function getImportFormat(specFormat: string, _apiType?: string): string | undefi
 interface RootApiResult {
   status: 'success' | 'skipped';
   specImported: boolean;
-  embeddedMcpApplied: boolean;
-}
-
-function getEmbeddedMcpProperties(
-  json: Record<string, unknown> | undefined
-): Record<string, unknown> | undefined {
-  const properties = json?.properties as Record<string, unknown> | undefined;
-  if (!properties) {
-    return undefined;
-  }
-
-  const embeddedProperties: Record<string, unknown> = {};
-  if (properties.mcpProperties !== undefined) {
-    embeddedProperties.mcpProperties = properties.mcpProperties;
-  }
-  if (properties.mcpTools !== undefined) {
-    embeddedProperties.mcpTools = properties.mcpTools;
-  }
-
-  return Object.keys(embeddedProperties).length > 0 ? embeddedProperties : undefined;
 }
 
 /**
@@ -153,33 +125,12 @@ async function publishRootApi(
       status: 'skipped',
       action: 'noop',
       specImported: false,
-      embeddedMcpApplied: false,
     };
   }
 
   // Apply overrides
   json = applyOverrides(descriptor, json, config.overrides);
 
-  const embeddedMcpProperties = getEmbeddedMcpProperties(
-    await store.readResource(config.sourceDir, {
-      type: ResourceType.McpServer,
-      nameParts: [...descriptor.nameParts],
-      workspace: descriptor.workspace,
-    })
-  );
-  const embeddedMcpApplied = embeddedMcpProperties !== undefined;
-  if (embeddedMcpApplied) {
-    const properties = json.properties as Record<string, unknown> | undefined;
-    json = {
-      ...json,
-      properties: {
-        ...(properties ?? {}),
-        ...embeddedMcpProperties,
-      },
-    };
-    logger.debug(`Merged MCP artifact into API payload for "${getNamePart(descriptor.nameParts, 0)}"`);
-  }
-
   // Try to read the specification file for this API
   let specImported = false;
   const specResult = await store.readContent(config.sourceDir, descriptor, 'specification');
@@ -227,7 +178,6 @@ async function publishRootApi(
     status: 'success',
     action: 'put',
     specImported,
-    embeddedMcpApplied,
   };
 }
 
@@ -288,8 +238,7 @@ async function publishApiChildren(
   context: ApimServiceContext,
   apiDescriptor: ResourceDescriptor,
   config: PublishConfig,
-  specImported: boolean = false,
-  embeddedMcpApplied: boolean = false
+  specImported: boolean = false
 ): Promise<void> {
   // List all resources from store
   const allDescriptors = await store.listResources(config.sourceDir);
@@ -299,8 +248,7 @@ async function publishApiChildren(
     (d) =>
       API_CHILD_TYPES.includes(d.type) &&
       getNamePart(d.nameParts, 0) === getNamePart(apiDescriptor.nameParts, 0) &&
-      !(specImported && SPEC_MANAGED_CHILD_TYPES.has(d.type)) &&
-      !(embeddedMcpApplied && d.type === ResourceType.McpServer)
+      !(specImported && SPEC_MANAGED_CHILD_TYPES.has(d.type))
   );
 
   // When a spec was imported, APIM auto-creates operations and schemas but loses

src/services/api-publisher.ts

@@ -101,9 +101,10 @@ describe('api-publisher', () => {
       );
     });
 
-    it('should merge embedded MCP artifact data into the root API payload', async () => {
+    it('should publish McpServer as a standard child resource without merging into root API payload', async () => {
       const client = createMockClient();
-      const store = createMockStore([]);
+      const mcpChild = { type: ResourceType.McpServer, nameParts: ['orders-api'] };
+      const store = createMockStore([mcpChild]);
       store.readResource.mockImplementation(async (_sourceDir: string, descriptor: ResourceDescriptor) => {
         if (descriptor.type === ResourceType.Api) {
           return { name: descriptor.nameParts[0] ?? '', properties: { type: 'mcp' } };
@@ -127,20 +128,13 @@ describe('api-publisher', () => {
 
       await publishApi(client, store, testContext, apiDescriptor, testConfig);
 
-      expect(client.putResource).toHaveBeenCalledWith(
-        testContext,
-        apiDescriptor,
-        expect.objectContaining({
-          properties: expect.objectContaining({
-            type: 'mcp',
-            mcpProperties: { serverUrl: 'https://example.com/mcp' },
-            mcpTools: [{ name: 'invokeTool' }],
-          }),
-        })
-      );
+      // Root API PUT should NOT contain MCP properties — they stay in the McpServer child
+      const [, , payload] = client.putResource.mock.calls[0] as [unknown, unknown, Record<string, unknown>];
+      const properties = payload.properties as Record<string, unknown> | undefined;
+      expect(properties).not.toHaveProperty('mcpProperties');
     });
 
-    it('should skip child MCP publish when MCP artifact was merged into the root API payload', async () => {
+    it('should include McpServer child in publish tasks (standard child, not skipped)', async () => {
       const client = createMockClient();
       const children = [
         { type: ResourceType.McpServer, nameParts: ['orders-api'] },
@@ -168,7 +162,8 @@ describe('api-publisher', () => {
       await publishApi(client, store, testContext, apiDescriptor, testConfig);
 
       const tasks = mockRunParallel.mock.calls[0][0] as Array<() => Promise<unknown>>;
-      expect(tasks).toHaveLength(1);
+      // Both McpServer and ApiPolicy should be included — McpServer is a standard child
+      expect(tasks).toHaveLength(2);
     });
 
     it('should return failed result when root API publish fails', async () => {

tests/unit/services/api-publisher.test.ts

@@ -326,6 +326,49 @@ describe('publish-service', () => {
       expect(result.totalPuts).toBe(2);
       expect(client.putResource).toHaveBeenCalledTimes(2);
     });
+
+    it('should not throw RangeError when ServicePolicy (nameParts=[]) is in the batch', async () => {
+      // ServicePolicy is a tier-2 top-level singleton with no name segments (nameParts: []).
+      // The isTopLevelSingleton() guard in executePuts must fire so that
+      // getNamePart(d.nameParts, 0) is never called for this resource.
+      const resources: ResourceDescriptor[] = [
+        { type: ResourceType.ServicePolicy, nameParts: [] },
+        { type: ResourceType.Api, nameParts: ['my-api'] },
+      ];
+
+      const client = createMockClient();
+      const store = createMockStore(resources);
+      // ServicePolicy is a policy type — its artifact is policy.xml, not JSON.
+      // Mock readContent so the policy publish path reaches putResource.
+      vi.mocked(store.readContent).mockImplementation(
+        async (_sourceDir, descriptor, contentType) => {
+          if (
+            descriptor.type === ResourceType.ServicePolicy &&
+            contentType === 'policy'
+          ) {
+            // Minimal valid APIM policy XML — content doesn't matter for this test,
+            // only that readContent returns something so publishPolicy reaches putResource.
+            return { content: '<policies><inbound/></policies>', format: 'xml' };
+          }
+          return undefined;
+        }
+      );
+
+      const config: PublishConfig = {
+        service: testContext,
+        sourceDir: '/source',
+        dryRun: false,
+        deleteUnmatched: false,
+        logLevel: LogLevel.INFO,
+      };
+
+      await expect(runPublish(client, store, config)).resolves.not.toThrow();
+      expect(client.putResource).toHaveBeenCalledWith(
+        testContext,
+        expect.objectContaining({ type: ResourceType.ServicePolicy }),
+        expect.anything(),
+      );
+    });
   });
 
   describe('pre-flight validation', () => {