fix: address second round of Copilot review comments

- getUserPhotoGated now throws for non-404 errors (401/403/5xx) so
  callers can distinguish a missing photo from a real API failure
- Log a debug message when photo loading fails instead of swallowing
  the error silently
- Merge transformer-pre-populated spec.members/spec.children with
  fetched Graph membership rather than overwriting them
- deriveRestLength now also excludes manual-trigger schedule objects
  (not just cron expressions) from being cast as HumanDuration

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: pillaris <pillaris@adobe.com>

plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.test.ts

@@ -416,7 +416,7 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
       expect(mockGetUserPhotoGated).toHaveBeenCalledWith(mockClient, 'u1', 120);
     });
 
-    it('continues processing remaining users when a photo load fails', async () => {
+    it('continues processing remaining users when a photo load fails and logs the error', async () => {
       mockRequestOnePage.mockResolvedValueOnce({
         items: [
           {
@@ -446,6 +446,10 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
 
       // Both users should still be emitted despite the photo failure
       expect(result.entities!).toHaveLength(2);
+      expect(logger.debug).toHaveBeenCalledWith(
+        expect.stringContaining('failed to load photo for user u1'),
+        expect.anything(),
+      );
     });
   });
 
@@ -731,6 +735,58 @@ describe('MicrosoftGraphIncrementalEntityProvider', () => {
       );
     });
 
+    it('merges transformer-pre-populated members with fetched membership', async () => {
+      (mockClient.getOrganization as jest.Mock).mockResolvedValue({
+        id: 'org-id',
+        displayName: 'My Org',
+      });
+      mockRequestOnePage.mockResolvedValueOnce({
+        items: [
+          { id: 'grp-1', displayName: 'Engineering', mail: 'eng@example.com' },
+        ],
+        nextLink: undefined,
+      });
+      (mockClient.getGroupMembers as jest.Mock).mockReturnValue(
+        asyncYield({
+          '@odata.type': '#microsoft.graph.user',
+          id: 'u2',
+          displayName: 'Bob',
+          userPrincipalName: 'bob@example.com',
+        }),
+      );
+
+      const provider = new MicrosoftGraphIncrementalEntityProvider({
+        id: 'default',
+        provider: baseProviderConfig as any,
+        logger,
+        groupTransformer: async group => {
+          const base = await import(
+            '@backstage/plugin-catalog-backend-module-msgraph'
+          ).then(m => m.defaultGroupTransformer(group));
+          if (!base) return undefined;
+          // Transformer pre-populates an extra member
+          base.spec = { ...base.spec, members: ['user:default/extra-user'] };
+          return base;
+        },
+      });
+
+      const result = await provider.next(makeContext(), groupsCursor);
+
+      const groupEntity = result.entities!.find(
+        e =>
+          e.entity.metadata.annotations?.[
+            MICROSOFT_GRAPH_GROUP_ID_ANNOTATION
+          ] === 'grp-1',
+      );
+      // Should contain both the transformer-set member and the fetched one
+      expect(groupEntity!.entity.spec?.members).toContain(
+        'user:default/extra-user',
+      );
+      expect(groupEntity!.entity.spec?.members).toContain(
+        'user:default/bob_example.com',
+      );
+    });
+
     it('passes group filter and search from provider config', async () => {
       const providerWithFilter = {
         ...baseProviderConfig,

plugins/catalog-backend-module-msgraph-incremental/src/MicrosoftGraphIncrementalEntityProvider.ts

@@ -319,8 +319,13 @@ export class MicrosoftGraphIncrementalEntityProvider
           if (provider.loadUserPhotos !== false) {
             try {
               userPhoto = await getUserPhotoGated(client, user.id!, 120);
-            } catch {
-              // Photo load failures are non-fatal
+            } catch (e) {
+              this.options.logger.debug(
+                `${this.getProviderName()}: failed to load photo for user ${
+                  user.id
+                }`,
+                { error: e },
+              );
             }
           }
 
@@ -451,11 +456,24 @@ export class MicrosoftGraphIncrementalEntityProvider
             }
           }
 
+          // Merge fetched membership with any members/children the transformer
+          // may have pre-populated, so custom transformers can augment the list.
+          const existingMembers = Array.isArray(entity.spec?.members)
+            ? (entity.spec.members as string[])
+            : [];
+          const existingChildren = Array.isArray(entity.spec?.children)
+            ? (entity.spec.children as string[])
+            : [];
+
           entities.push({
             locationKey: `msgraph-org-provider:${this.options.id}`,
             entity: withLocations(this.options.id, {
               ...entity,
-              spec: { ...entity.spec, members: userRefs, children: childRefs },
+              spec: {
+                ...entity.spec,
+                members: [...new Set([...existingMembers, ...userRefs])],
+                children: [...new Set([...existingChildren, ...childRefs])],
+              },
             }),
           });
         }),

plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.test.ts

@@ -225,7 +225,7 @@ describe('getUserPhotoGated', () => {
 
   afterEach(() => jest.resetAllMocks());
 
-  it('returns undefined when the photo check returns non-200', async () => {
+  it('returns undefined when the photo check returns 404', async () => {
     (client.requestApi as jest.Mock).mockResolvedValue({
       status: 404,
     } as Response);
@@ -236,6 +236,17 @@ describe('getUserPhotoGated', () => {
     expect(client.getUserPhotoWithSizeLimit).not.toHaveBeenCalled();
   });
 
+  it('throws for non-404 error responses so callers can distinguish real errors', async () => {
+    (client.requestApi as jest.Mock).mockResolvedValue({
+      status: 403,
+    } as Response);
+
+    await expect(getUserPhotoGated(client, 'user-id', 120)).rejects.toThrow(
+      'Unexpected status 403 when checking photo for user user-id',
+    );
+    expect(client.getUserPhotoWithSizeLimit).not.toHaveBeenCalled();
+  });
+
   it('returns the photo data URI when the check passes', async () => {
     (client.requestApi as jest.Mock).mockResolvedValue({
       status: 200,

plugins/catalog-backend-module-msgraph-incremental/src/clientHelpers.ts

@@ -85,6 +85,9 @@ export async function requestOnePage<T>(
  * Like `getUserPhotoWithSizeLimit` but skips the size-listing call for users
  * with no photo. For users without a photo: 1 fast check call. For users with
  * a photo: 1 check + the normal size-limited fetch (2 more calls).
+ *
+ * Returns `undefined` only for 404 (no photo assigned). Throws for any other
+ * non-200 status so callers can distinguish "no photo" from real errors.
  */
 export async function getUserPhotoGated(
   client: MicrosoftGraphClient,
@@ -92,6 +95,11 @@ export async function getUserPhotoGated(
   maxSize: number,
 ): Promise<string | undefined> {
   const check = await client.requestApi(`users/${userId}/photo`);
-  if (check.status !== 200) return undefined;
+  if (check.status === 404) return undefined;
+  if (check.status !== 200) {
+    throw new Error(
+      `Unexpected status ${check.status} when checking photo for user ${userId}`,
+    );
+  }
   return await client.getUserPhotoWithSizeLimit(userId, maxSize);
 }

plugins/catalog-backend-module-msgraph-incremental/src/module/catalogModuleMicrosoftGraphIncrementalEntityProvider.ts

@@ -231,13 +231,20 @@ function deriveRestLength(
   logger: LoggerService,
 ): HumanDuration {
   const freq = providerConfig.schedule?.frequency;
-  if (freq && typeof freq === 'object' && !('cron' in freq)) {
+  // Only treat plain duration objects as restLength — exclude cron expressions
+  // and any other non-duration schedule types (e.g. manual triggers).
+  if (
+    freq &&
+    typeof freq === 'object' &&
+    !('cron' in freq) &&
+    !('trigger' in freq)
+  ) {
     return freq as HumanDuration;
   }
   if (freq) {
     logger.warn(
       `MicrosoftGraphIncrementalEntityProvider:${providerConfig.id}: ` +
-        `schedule.frequency is a cron expression; cannot derive restLength from it. ` +
+        `schedule.frequency is not a duration-based schedule; cannot derive restLength from it. ` +
         `Defaulting restLength to 8 hours.`,
     );
   }