PR Review Summary

4 Key Findings | Risk: Medium

🟠⚠️ Major (2) 🟠⚠️

🟠 1) Agent.ts:3265-3278 Empty data URL fallback creates invalid image when blob URI resolution fails

Issue: When blobUriToProxyUrl returns null for a malformed blob key, the code falls back to creating a data URL with no actual base64 data: data:image/*;base64,. This produces an invalid image that will either be silently ignored by the AI SDK or cause confusing errors far removed from the root cause.

Why: Users will see images mysteriously disappear from agent responses with no indication of what went wrong. The resolveMessageBlobUris function in the same module correctly filters out parts when resolution fails (returns []), but this code creates a malformed placeholder instead. The malformed blob URI is logged at error level in blobUriToProxyUrl, but the Agent continues as if nothing happened.

Fix: Skip the image part when resolution fails, matching the pattern in resolveMessageBlobUris:

if ('uri' in file && file.uri) {
  if (isBlobUri(file.uri)) {
    const resolvedUri = blobUriToProxyUrl(file.uri);
    if (!resolvedUri) {
      // Skip malformed blob URIs - already logged by blobUriToProxyUrl
      continue;
    }
    imageValue = new URL(resolvedUri);
  } else {
    imageValue = new URL(file.uri);
  }
} else {
  imageValue = `data:${file.mimeType || 'image/*'};base64,${file.bytes}`;
}

Refs:

• Agent.ts:3265-3278
• resolve-blob-uris.ts:34-35 — existing pattern that filters out malformed parts

🟠 2) executionHandler.ts, relationTools.ts Missing test coverage for core image preservation logic

Issue: The PR adds minimal test changes (2 assertions updating expected message structure) but introduces significant new behavior:

1. The originalNonTextParts extraction logic (lines 238-240) that preserves file and data parts across transfer iterations has no dedicated test coverage
2. The delegation tool's userImageParts parameter threading (line 393) is not tested — existing tests don't pass or verify image parts flow through to A2A messages

Why: These are the core fixes for the PR's stated purpose. If this filter logic regresses (e.g., someone changes the condition or removes the spread), images would silently be dropped after the first transfer or during delegation. Users would experience image loss when their request gets transferred between agents — exactly the bug this PR addresses.

Fix: Add targeted tests for the new behavior:

For transfer loop preservation:

it('should preserve file parts across transfer iterations', () => {
  const messageParts = [
    { kind: 'text', text: 'Hello' },
    { kind: 'file', file: { uri: 'blob://t/p/c/m/img.png', mimeType: 'image/png' } },
  ];
  
  const originalNonTextParts = messageParts.filter(
    (p) => p.kind === 'file' || p.kind === 'data'
  );
  
  expect(originalNonTextParts).toHaveLength(1);
  
  // Simulate second iteration
  const secondIterationParts = [
    { kind: 'text', text: 'Updated message' },
    ...originalNonTextParts
  ];
  expect(secondIterationParts).toHaveLength(2);
  expect(secondIterationParts[1].kind).toBe('file');
});

For delegation image threading:

it('should include userImageParts in delegation message', async () => {
  const userImageParts = [{
    kind: 'file' as const,
    file: { uri: 'blob://t/p/c/m/img.png', mimeType: 'image/png' }
  }];
  
  const tool = createDelegateToAgentTool({
    ...getDelegateParams(),
    userImageParts,
  });
  
  await tool.execute({ message: 'Analyze this image' }, mockToolCallOptions);
  
  expect(mockSendMessage).toHaveBeenCalledWith(
    expect.objectContaining({
      message: expect.objectContaining({
        parts: expect.arrayContaining([
          expect.objectContaining({ kind: 'file' })
        ])
      })
    })
  );
});

Refs:

• executionHandler.ts:238-291
• relationTools.ts:391-393

// Findings posted as inline comments:

• 🟠 Major: Agent.ts:3265-3278 Empty data URL fallback

🟡 Minor (2) 🟡

🟡 1) image-upload-helpers.ts:14 PersistedMessageResult interface not exported

Issue: The PersistedMessageResult interface is not exported, but follows the codebase pattern for Result types which are typically exported.
Why: Limits reusability and testability — other modules can't type the return value without duplication.
Fix: Add export keyword to the interface.
Refs: image-upload-helpers.ts:14-17

🟡 2) resolve-blob-uris.ts:8-22 blobUriToProxyUrl null path not tested

Issue: The newly extracted blobUriToProxyUrl function can return null for malformed blob keys, but there are no tests for this edge case.
Why: This null path is now exercised in the hot path (Agent.buildUserMessageContent) and could produce unexpected behavior if the handling changes.
Fix: Add unit tests for malformed key scenarios.
Refs: resolve-blob-uris.ts:8-22

// Findings posted as inline comments:

• 🟡 Minor: image-upload-helpers.ts:14 Export PersistedMessageResult interface

💭 Consider (1) 💭

💭 1) image-upload-helpers.ts:43-51 Error logs could include more context

Issue: When image upload fails, the error log only includes messageId, not the full context (tenantId, projectId, conversationId).
Why: Makes incident correlation harder during debugging.
Fix: Include the full ctx object in error logs.
Refs: image-upload-helpers.ts:43-51

Summary: This PR correctly fixes image preservation through agent transfers and delegations — a meaningful bug fix. The architecture is sound: threading userImageParts via closures is consistent with existing patterns for credentialStoreRegistry and sessionId. The main concerns are (1) the empty data URL fallback when blob URI resolution fails should skip the image instead of creating a malformed placeholder, and (2) the core image preservation logic needs test coverage to prevent regressions. Both are addressable and don't block the fix.