feat(images): enable multi-turn conversations about image context parts#2604
feat(images): enable multi-turn conversations about image context parts#2604mike-inkeep wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Enables multi-turn image conversations by hydrating blob-stored images back into data URLs when replaying conversation history, so models that support images can "see" previously uploaded images across turns. Key changes:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach of splitting hydrated image messages from text-only messages and sending them as separate ModelMessage[] entries is sound. There are a few issues that should be addressed before merge — a leftover console.log, an ordering bug, and dropped text context for hydrated messages.
| conversationHistory = nonHydrated; | ||
| } | ||
|
|
||
| console.log('conversationHistoryWithFileData', conversationHistoryWithFileData); |
There was a problem hiding this comment.
Leftover console.log — must be removed before merge. Use logger.debug if debug output is still needed.
| await Promise.all( | ||
| messages.map(async (msg) => { | ||
| const content = msg.content; | ||
| if (!content?.parts?.length) { | ||
| nonHydrated.push(msg); | ||
| return; | ||
| } | ||
|
|
||
| let hasHydratedParts = false; | ||
| const parts = await Promise.all( | ||
| content.parts.map(async (part: NonNullable<MessageSelect['content']['parts']>[number]) => { | ||
| if (part.kind !== 'file' || typeof part.data !== 'string' || !isBlobUri(part.data)) { | ||
| return part; | ||
| } | ||
| const mimeType = | ||
| typeof part.metadata?.mimeType === 'string' ? part.metadata.mimeType : undefined; | ||
| const dataUrl = await hydrate(part.data, mimeType); | ||
| if (dataUrl) { | ||
| hasHydratedParts = true; | ||
| return { ...part, data: dataUrl }; | ||
| } | ||
| return part; | ||
| }) | ||
| ); | ||
|
|
||
| const processedMsg = { ...msg, content: { ...content, parts } }; | ||
| if (hasHydratedParts) { | ||
| hydrated.push(processedMsg); | ||
| } else { | ||
| nonHydrated.push(processedMsg); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Ordering bug. Promise.all runs all callbacks concurrently and the push calls into hydrated/nonHydrated happen whenever each message's inner Promise.all (for parts) resolves — not in the original array order. If downstream code or the model relies on chronological ordering of image messages, this will silently reorder them.
Consider mapping to Promise<{ index, msg, wasHydrated }>, awaiting all, then partitioning by index order.
| if (conversationHistoryWithFileData?.length) { | ||
| conversationHistoryWithFileData.forEach((msg) => { | ||
| const content = (msg.content?.parts ?? []).flatMap((part) => { | ||
| if (part.kind !== 'file' || typeof part.data !== 'string') { | ||
| return []; | ||
| } | ||
|
|
||
| return [ | ||
| { | ||
| type: 'image' as const, | ||
| image: part.data, | ||
| }, | ||
| ]; | ||
| }); | ||
|
|
||
| if (content?.length && content.length > 0) { | ||
| messages.push({ role: 'user', content }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Text context is dropped. Hydrated messages only extract file parts — all text parts from the same message are discarded via flatMap. The model will receive image content with no accompanying text from that original message (e.g. a user's description of the image). Consider also including text parts so the model can associate each image with its context.
There was a problem hiding this comment.
This was true in the original code but has been fixed in the "Review feedback" commit. Looking at the current buildInitialMessages (line ~189), the reduce now handles both text and file parts — text parts are included as { type: 'text', text: part.text } alongside the image parts. So text context is preserved in the hydrated messages now.
| const conversationHistoryString = formatMessagesAsConversationHistory(conversationHistory); | ||
|
|
||
| const conversationHistoryTokens = estimateTokens(conversationHistoryString); |
There was a problem hiding this comment.
Token estimation gap: conversationHistoryTokens is computed only from conversationHistoryString. The hydrated image messages — potentially large base64 data URLs — are excluded from the context breakdown entirely. Context window management won't account for image data.
| async function hydrateConversationHistoryBlobParts( | ||
| messages: MessageSelect[], | ||
| hydrate: HydrateBlobToDataUrl | ||
| ): Promise<{ hydrated: MessageSelect[]; nonHydrated: MessageSelect[] }> { | ||
| const hydrated: MessageSelect[] = []; | ||
| const nonHydrated: MessageSelect[] = []; | ||
|
|
||
| await Promise.all( | ||
| messages.map(async (msg) => { | ||
| const content = msg.content; | ||
| if (!content?.parts?.length) { | ||
| nonHydrated.push(msg); | ||
| return; | ||
| } | ||
|
|
||
| let hasHydratedParts = false; | ||
| const parts = await Promise.all( | ||
| content.parts.map(async (part: NonNullable<MessageSelect['content']['parts']>[number]) => { | ||
| if (part.kind !== 'file' || typeof part.data !== 'string' || !isBlobUri(part.data)) { | ||
| return part; | ||
| } | ||
| const mimeType = | ||
| typeof part.metadata?.mimeType === 'string' ? part.metadata.mimeType : undefined; | ||
| const dataUrl = await hydrate(part.data, mimeType); | ||
| if (dataUrl) { | ||
| hasHydratedParts = true; | ||
| return { ...part, data: dataUrl }; | ||
| } | ||
| return part; | ||
| }) | ||
| ); | ||
|
|
||
| const processedMsg = { ...msg, content: { ...content, parts } }; | ||
| if (hasHydratedParts) { | ||
| hydrated.push(processedMsg); | ||
| } else { | ||
| nonHydrated.push(processedMsg); | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| return { hydrated, nonHydrated }; | ||
| } |
There was a problem hiding this comment.
No test coverage. hydrateConversationHistoryBlobParts is the core of this feature but has no dedicated tests. Key scenarios to cover: successful blob hydration, messages with mixed blob/non-blob parts, failed hydration (hydrate() returns null), and ordering preservation.
| const summaryMessage: MessageSelect = { | ||
| ...messageHistory[0], |
There was a problem hiding this comment.
...messageHistory[0] spreads the entire MessageSelect (including content, metadata, conversationId, taskId, etc.) into the summary, then overwrites only a few fields. This leaks unrelated data from the first message into the summary. Consider explicitly constructing only the required fields.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
conversation-history.ts:135Debug console.log left in production code — outputs potentially sensitive image/message data to stdout
🟠⚠️ Major (4) 🟠⚠️
🟠 1) blob-to-data-url.ts Missing test coverage for new critical functionality
Issue: The new createBlobToDataUrlHydrator function (lines 11-38) has zero unit test coverage. This function handles blob storage downloads, base64 encoding, MIME type resolution, caching, and error handling — all critical paths for the image replay feature.
Why: Without tests: (1) A regression in base64 encoding could cause model API failures. (2) The cache could fail silently, causing repeated downloads that degrade performance. (3) Error handling caches null but behavior isn't verified. (4) MIME type fallback chain isn't tested.
Fix: Add a dedicated test file agents-api/src/__tests__/run/services/blob-storage/blob-to-data-url.test.ts with tests for:
- Successful blob-to-data-url conversion
- Cache hit behavior on repeated calls
- MIME type precedence (downloaded.contentType → fallbackMimeType → 'application/octet-stream')
- Error handling when storage.download fails
- Logger.warn called on failure
Refs:
Inline Comments:
- 🟠 Major:
conversation-history.ts:25-57Race condition causes non-deterministic message ordering in hydration - 🟠 Major:
blob-to-data-url.ts:15In-memory cache without size limits risks memory exhaustion - 🟠 Major:
blob-to-data-url.ts:29-36Silent failure pattern may cause confusing model behavior
🟡 Minor (0) 🟡
No minor issues.
💭 Consider (3) 💭
💭 1) conversation-history.ts:18-60 Add tests for hydrateConversationHistoryBlobParts
Issue: The new hydrateConversationHistoryBlobParts function has complex async logic with multiple edge cases that aren't covered by existing tests.
Why: The function has 4+ conditional branches and mixed parts handling that could regress silently.
Fix: Consider adding tests for: messages without parts, non-blob URIs, hydrator returning null, messages with mixed blob/non-blob parts.
💭 2) conversation-history.ts:25 Add concurrency limit for parallel blob downloads
Issue: Promise.all downloads all blob parts in parallel without a concurrency limit.
Why: For conversations with many images (e.g., 20+), this fires concurrent requests that could exhaust connection pools or trigger rate limiting.
Fix: Consider using p-limit with 5-10 concurrent downloads: import pLimit from 'p-limit'; const limit = pLimit(5);
💭 3) blob-to-data-url.ts:22-28 Add blob size validation before base64 encoding
Issue: Downloaded blob data is converted to base64 without checking size. A large blob (50MB) would require ~67MB heap for the base64 string.
Why: Edge case, but could cause memory spikes on unusually large images.
Fix: Add size check: if (downloaded.data.length > MAX_SIZE) { logger.warn(...); return null; }
🚫 REQUEST CHANGES
Summary: This PR adds valuable functionality for multi-turn image conversations, but has a critical debug statement that must be removed before merge. The race condition in message ordering is also a significant concern that could cause non-deterministic behavior. Additionally, the new blob hydration code path lacks test coverage for a feature that's critical to the user experience.
The architecture is sound — the separation of hydrated vs non-hydrated messages, the caching strategy, and the factory pattern all follow good patterns. Once the critical/major issues are addressed, this should be ready to ship.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
types.ts |
Function signature mismatch | Assessed as working correctly — index.ts wrapper provides the zero-arg signature |
conversation-history.ts:179-194 |
Silent filtering of non-file parts | Intentional design — image-only replay is the feature goal |
local-provider.ts:109-125 |
No timeout on local file reads | Dev-only concern, out of PR scope |
Agent.ts:114 |
Hydrator cache not cleared on cleanup | Agent is cleaned up per-request, cache released via GC |
conversations.ts:163-164 |
Unsafe as unknown as any cast |
Pre-existing pattern for legacy metadata format, not introduced by this PR |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
2 | 0 | 0 | 0 | 2 | 0 | 0 |
pr-review-tests |
4 | 1 | 1 | 0 | 0 | 0 | 2 |
pr-review-types |
6 | 0 | 0 | 0 | 0 | 0 | 6 |
pr-review-errors |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-consistency |
6 | 0 | 0 | 0 | 1 | 0 | 5 |
pr-review-sre |
6 | 0 | 2 | 0 | 2 | 0 | 2 |
| Total | 27 | 1 | 3 | 0 | 6 | 0 | 17 |
Note: High discard rate due to significant overlap — 5 reviewers independently flagged the console.log issue, merged into single Critical finding.
| conversationHistory = nonHydrated; | ||
| } | ||
|
|
||
| console.log('conversationHistoryWithFileData', conversationHistoryWithFileData); |
There was a problem hiding this comment.
🔴 CRITICAL: Debug console.log left in production code
Issue: This console.log will output potentially large base64-encoded image data and message content to stdout on every request with images.
Why: This creates (1) excessive log volume and storage costs, (2) potential PII/data exposure of user-uploaded images in logs, (3) performance overhead from serialization. All 5 reviewers flagged this.
Fix: (1-click apply)
| console.log('conversationHistoryWithFileData', conversationHistoryWithFileData); |
Refs:
| await Promise.all( | ||
| messages.map(async (msg) => { | ||
| const content = msg.content; | ||
| if (!content?.parts?.length) { | ||
| nonHydrated.push(msg); | ||
| return; | ||
| } | ||
|
|
||
| let hasHydratedParts = false; | ||
| const parts = await Promise.all( | ||
| content.parts.map(async (part: NonNullable<MessageSelect['content']['parts']>[number]) => { | ||
| if (part.kind !== 'file' || typeof part.data !== 'string' || !isBlobUri(part.data)) { | ||
| return part; | ||
| } | ||
| const mimeType = | ||
| typeof part.metadata?.mimeType === 'string' ? part.metadata.mimeType : undefined; | ||
| const dataUrl = await hydrate(part.data, mimeType); | ||
| if (dataUrl) { | ||
| hasHydratedParts = true; | ||
| return { ...part, data: dataUrl }; | ||
| } | ||
| return part; | ||
| }) | ||
| ); | ||
|
|
||
| const processedMsg = { ...msg, content: { ...content, parts } }; | ||
| if (hasHydratedParts) { | ||
| hydrated.push(processedMsg); | ||
| } else { | ||
| nonHydrated.push(processedMsg); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🟠 MAJOR: Race condition causes non-deterministic message ordering
Issue: Promise.all processes messages concurrently, but results are pushed to shared arrays (hydrated, nonHydrated) inside async callbacks. This causes messages to appear in arbitrary order based on which async operations complete first.
Why: Conversation history order is semantically important. Downstream code like buildInitialMessages will receive messages in random order, potentially confusing the model about conversation flow.
Fix: Use Promise.all to return indexed results, then partition while preserving order:
async function hydrateConversationHistoryBlobParts(
messages: MessageSelect[],
hydrate: HydrateBlobToDataUrl
): Promise<{ hydrated: MessageSelect[]; nonHydrated: MessageSelect[] }> {
const results = await Promise.all(
messages.map(async (msg, index) => {
const content = msg.content;
if (!content?.parts?.length) {
return { index, msg, wasHydrated: false };
}
let hasHydratedParts = false;
const parts = await Promise.all(
content.parts.map(async (part) => {
if (part.kind !== 'file' || typeof part.data !== 'string' || !isBlobUri(part.data)) {
return part;
}
const mimeType = typeof part.metadata?.mimeType === 'string' ? part.metadata.mimeType : undefined;
const dataUrl = await hydrate(part.data, mimeType);
if (dataUrl) {
hasHydratedParts = true;
return { ...part, data: dataUrl };
}
return part;
})
);
return {
index,
msg: { ...msg, content: { ...content, parts } },
wasHydrated: hasHydratedParts
};
})
);
// Sort by original index to preserve order
results.sort((a, b) => a.index - b.index);
const hydrated: MessageSelect[] = [];
const nonHydrated: MessageSelect[] = [];
for (const { msg, wasHydrated } of results) {
(wasHydrated ? hydrated : nonHydrated).push(msg);
}
return { hydrated, nonHydrated };
}Refs:
- MDN Promise.all — "The returned promise fulfills when all of the input's promises fulfill... with an array of the fulfillment values, in the order of the promises passed"
| storage: BlobStorageProvider, | ||
| fromBlobUri: (uri: string) => string | ||
| ): HydrateBlobToDataUrl { | ||
| const cache = new Map<string, string | null>(); |
There was a problem hiding this comment.
🟠 MAJOR: In-memory cache without size limits risks memory exhaustion
Issue: The Map<string, string | null> cache has no eviction policy. Each cached data URL stores the full base64-encoded image (~33% larger than original binary). In conversations with many images, this cache grows unbounded.
Why: While the hydrator is created per-Agent and Agents are cleaned up per-request, during a single multi-turn conversation with many images, memory can spike significantly. A conversation with 10 images at 5MB each = ~65MB of cached strings just for this map.
Fix: Consider one of these approaches:
- LRU cache with size limit (recommended):
import { LRUCache } from 'lru-cache';
const cache = new LRUCache<string, string | null>({
max: 50, // max entries
maxSize: 50 * 1024 * 1024, // 50MB total
sizeCalculation: (value) => value?.length ?? 1,
});- Simple size limit:
const MAX_CACHE_SIZE_BYTES = 50 * 1024 * 1024;
let cacheSize = 0;
// Before caching:
if (cacheSize + dataUrl.length > MAX_CACHE_SIZE_BYTES) {
cache.clear();
cacheSize = 0;
}
cache.set(blobUri, dataUrl);
cacheSize += dataUrl.length;Refs:
There was a problem hiding this comment.
A size limit also seems like a valid point to make, we dont want people to abuse it? Maybe we could have an env for max image bytes per agent session?
There was a problem hiding this comment.
An env-based max is a reasonable approach. A few thoughts on the design:
-
Scope: A per-session byte limit (total cached data URLs) is simpler and more predictable than per-image limits. Something like
INKEEP_MAX_IMAGE_CACHE_BYTESwith a sensible default (e.g. 50MB) would cover most cases without config burden. -
Eviction vs. rejection: When the limit is hit, the simplest approach is to stop caching new entries (still hydrate, just don't cache) rather than evicting old ones. An LRU adds complexity that may not be warranted given the per-request lifecycle.
-
Alternatively: Since the cache lives only for a single request (Agent is per-request), the practical risk is limited to conversations with many large images in a single turn. A simple entry count cap (e.g. max 20 cached images) might be sufficient and avoids the overhead of tracking byte sizes.
| } catch (error) { | ||
| logger.warn( | ||
| { error, blobUri }, | ||
| 'Failed to hydrate blob URI for multimodal conversation history replay' | ||
| ); | ||
| cache.set(blobUri, null); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Silent failure pattern may cause confusing model behavior
Issue: The catch block handles ALL errors identically — caches null and returns null. This means the original blob:// URI remains in the message, which the model will receive as an invalid image URL.
Why: When hydration fails, the model receives a message with image: "blob://..." instead of actual image data. This could cause: (1) model API errors, (2) model confusion trying to interpret the string, (3) silent degradation where users don't understand why the model "can't see" previous images.
Fix: Consider differentiating error types and improving visibility:
} catch (error) {
const isNotFound = error instanceof Error &&
(error.message.includes('NoSuchKey') ||
error.message.includes('404') ||
error.message.includes('not found'));
if (isNotFound) {
logger.info({ blobUri }, 'Blob no longer exists, excluding from history');
} else {
logger.error({ error, blobUri }, 'Failed to hydrate blob - check storage configuration');
}
cache.set(blobUri, null);
return null;
}Also consider: instead of keeping the unhydrated part in the message, filter it out entirely so the model doesn't receive invalid URIs.
Refs:
There was a problem hiding this comment.
I dont think the model could do much with failed hydration since this is a purely backend operation. Maybe we should have some replacement for it, but dont think model needs to know how it failed. More that an image was supposed to be there?
There was a problem hiding this comment.
Agreed — the model doesn't need to know why hydration failed, just that an image was expected there. A good pattern would be to replace the failed part with a text placeholder like [Image unavailable] so the model understands there was supposed to be visual context without receiving an invalid blob:// URI. This is cleaner than the current approach where null is returned and the part is kept with its original blob URI (which the model can't interpret).
In buildInitialMessages, parts where part.data is still a blob URI after hydration could be mapped to { type: 'text', text: '[Image unavailable]' } instead.
mike-inkeep
force-pushed
the
images/hydrate_image_history
branch
from
ce601b8 to
acb773c
Compare
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
🔴❗ Critical (0) ❗🔴
None — the console.log issue from the prior review was fixed in the "Review feedback" commit.
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
conversation-history.ts:30Unbounded parallel blob downloads may exhaust connections - 🟠 Major:
conversations.ts:430Return type change breaks established API contract
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
conversation-history.ts:195-199Hardcoded assumption that all file parts are images
💭 Consider (0) 💭
Items from prior review either addressed or carried forward as Pending Recommendations.
🕐 Pending Recommendations (3)
Prior issues still unresolved:
- 🟠
blob-to-data-url.ts:15In-memory cache without size limits risks memory exhaustion - 🟠
blob-to-data-url.ts:29-36Silent failure pattern may cause confusing model behavior - 🟠
blob-to-data-url.tsMissing test coverage for new critical functionality
💡 APPROVE WITH SUGGESTIONS
Summary: The "Review feedback" commit addressed the 4 most critical issues from the prior review:
- ✅ Removed debug console.log
- ✅ Fixed race condition in message ordering (changed from push-based to map-then-filter)
- ✅ Fixed token estimation to include hydrated file data
- ✅ Fixed text context being dropped from hydrated messages (now includes both text and file parts)
The architecture is sound and the PR is ready for merge with the understanding that test coverage and concurrency limiting are follow-up items. The new inline comments highlight:
- Concurrency limit for blob downloads (matches existing upload pattern)
- API naming consideration for the return type change
- Type safety for non-image file types (low risk given feature scope)
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
conversations.ts:163-165 |
Unsafe type assertion for compressionType | Pre-existing pattern for backward compatibility, not introduced by this PR |
conversations.ts:272-275 |
getScopedHistory returns empty on error | Pre-existing behavior, out of PR scope |
Agent.ts:114 |
Hydrator cache not cleared on cleanup | Agent is cleaned up per-request, cache released via GC |
conversation-history.ts:21-59 |
Partition invariant not typed | Logic is correct even if type doesn't encode invariant; medium confidence |
blob-to-data-url.ts:6-9 |
Hydration result type ambiguity | Pragmatic tradeoff for the feature scope; low confidence |
conversation-history.ts |
Function signature inconsistency | Mixed pattern is acceptable; generation-layer vs data-layer distinction |
conversations.ts:916 |
formatMessages export visibility | Informational only - appropriate API expansion |
core/conversations.ts:251-275 |
Inline message construction | Low confidence - minor code clarity issue |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
5 | 0 | 0 | 0 | 1 | 0 | 4 |
pr-review-tests |
4 | 0 | 0 | 0 | 0 | 3 | 1 |
pr-review-sre |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-errors |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-types |
5 | 0 | 0 | 0 | 1 | 0 | 4 |
| Total | 21 | 0 | 0 | 0 | 3 | 3 | 15 |
Note: High discard rate due to (1) issues already raised in prior review routed to Pending Recommendations, (2) pre-existing patterns not introduced by this PR, (3) low-confidence findings filtered out.
| } | ||
|
|
||
| let hasHydratedParts = false; | ||
| const parts = await Promise.all( |
There was a problem hiding this comment.
🟠 MAJOR: Unbounded parallel blob downloads may exhaust connections
Issue: The nested Promise.all at lines 22-49 downloads all blob URIs concurrently without any concurrency limit. With up to 50 messages in conversation history, each potentially containing multiple image parts, this could trigger dozens of concurrent downloads.
Why: The upload path uses FILE_UPLOAD_CONCURRENCY = 3 (image-upload.ts:13) for good reason — to avoid connection pool exhaustion and rate limiting. Under load, unbounded downloads could:
- Exhaust connection pools to S3/Vercel Blob storage
- Trigger rate limiting (especially Vercel Blob with per-second limits)
- Create memory pressure from buffering many large images simultaneously
Fix: Apply the same worker pool pattern used in image-upload.ts with 3-5 concurrent downloads. Consider using p-limit:
import pLimit from 'p-limit';
const limit = pLimit(3);
const parts = await Promise.all(
content.parts.map((part) => limit(async () => {
// existing hydration logic
}))
);Refs:
There was a problem hiding this comment.
I think bounding by a concurrency measure is valid
There was a problem hiding this comment.
Agreed. Using p-limit with a concurrency of 3–5 (matching the FILE_UPLOAD_CONCURRENCY = 3 pattern in image-upload.ts) would be the cleanest approach. The inner Promise.all over parts within a single message is fine unbounded (a message typically has 1–3 parts), but the outer Promise.all over all messages should be bounded since conversation history can have dozens of messages with images.
| streamRequestId?: string; | ||
| fullContextSize?: number; | ||
| }): Promise<string> { | ||
| }): Promise<MessageSelect[]> { |
There was a problem hiding this comment.
🟠 MAJOR: Return type change breaks established API contract
Issue: getConversationHistoryWithCompression changed from Promise<string> to Promise<MessageSelect[]>. This is a breaking change to an internal API that requires callers to now call formatMessagesAsConversationHistory() separately.
Why: The sibling function getFormattedConversationHistory (lines 319-401) returns Promise<string>. The naming convention get*ConversationHistory* suggested a string return type. This creates a split-world pattern where some history functions return strings and others return message arrays.
Fix: Consider one of:
- Rename to
getConversationMessagesWithCompressionto indicate it returns messages not a formatted string - Document this as an intentional API evolution in a code comment
- Consider whether
getFormattedConversationHistoryshould follow the same pattern for consistency
Refs:
- getFormattedConversationHistory:319-401 — returns
Promise<string>
| if (part.kind === 'file' && typeof part.data === 'string') { | ||
| // Temporarily hard code to image. Must update when we add other file types. | ||
| acc.push({ type: 'image', image: part.data }); | ||
| return acc; | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Hardcoded assumption that all file parts are images
Issue: The comment acknowledges this ("Temporarily hard code to image. Must update when we add other file types."), but the type system allows any kind: 'file' part to reach this code path, including PDFs or other binary data.
Why: If a non-image file part is stored in conversation history, it will be sent to the model as an image, which will likely cause API errors or undefined behavior.
Fix: Add a type guard for image MIME types:
| if (part.kind === 'file' && typeof part.data === 'string') { | |
| // Temporarily hard code to image. Must update when we add other file types. | |
| acc.push({ type: 'image', image: part.data }); | |
| return acc; | |
| } | |
| if (part.kind === 'file' && typeof part.data === 'string') { | |
| const mimeType = part.metadata?.mimeType; | |
| if (typeof mimeType === 'string' && mimeType.startsWith('image/')) { | |
| acc.push({ type: 'image', image: part.data }); | |
| } | |
| return acc; | |
| } |
Refs:
- PR title — Feature is specifically for "image context parts"
Ito Test Report ❌8 test cases ran. 7 passed, 1 failed. The run confirms stable behavior for the validated resilience and adversarial checks, and code review identifies one likely real authorization/isolation defect in conversation scoping. Findings are based on runtime evidence cross-checked against production code paths. ✅ Passed (7)
❌ Failed (1)
Cross-project conversationId isolation – Failed
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low | Delta: Merge from main only
Delta Analysis
Since the last review (acb773c2e9e7), the only change is a merge commit from main:
| Check | Status |
|---|---|
| PR implementation files changed | ✅ None — all 10 files unchanged |
| Merge conflicts | ✅ None |
| Breaking changes from main | ✅ None affecting this feature |
| Test coverage added | ❌ Still missing (see Pending Recommendations) |
The merge brought in the App Credentials feature (#2501) and security dependency bumps, but none of these affect the image hydration implementation.
🕐 Pending Recommendations (3)
These issues from the prior review remain unaddressed:
- 🟠
blob-to-data-url.ts:15In-memory cache without size limits risks memory exhaustion - 🟠
blob-to-data-url.ts:29-36Silent failure pattern may cause confusing model behavior - 🟠
blob-to-data-url.tsMissing test coverage forcreateBlobToDataUrlHydrator
Additionally from the second review:
- 🟠
conversation-history.ts:30Unbounded parallel blob downloads may exhaust connections - 🟡
conversation-history.ts:195-199Hardcoded assumption that all file parts are images
💡 APPROVE WITH SUGGESTIONS
Summary: No new issues introduced. The merge from main was clean with no conflicts affecting the image hydration feature. The prior review's verdict of APPROVE WITH SUGGESTIONS remains valid.
Ready to merge with the understanding that the pending recommendations (test coverage, concurrency limiting, cache bounds) can be addressed in follow-up work. The core functionality is sound and addresses a real user need for multi-turn image conversations.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| None dispatched | 0 | 0 | 0 | 0 | 0 | 5 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 5 | 0 |
Note: Delta review — no reviewers dispatched as PR files are unchanged since the last review.
tim-inkeep
left a comment
tim-inkeep
left a comment
There was a problem hiding this comment.
Have some comments that want your thoughts on, also wanted to check which part is the next step for you from this?
| storage: BlobStorageProvider, | ||
| fromBlobUri: (uri: string) => string | ||
| ): HydrateBlobToDataUrl { | ||
| const cache = new Map<string, string | null>(); |
There was a problem hiding this comment.
A size limit also seems like a valid point to make, we dont want people to abuse it? Maybe we could have an env for max image bytes per agent session?
| } catch (error) { | ||
| logger.warn( | ||
| { error, blobUri }, | ||
| 'Failed to hydrate blob URI for multimodal conversation history replay' | ||
| ); | ||
| cache.set(blobUri, null); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I dont think the model could do much with failed hydration since this is a purely backend operation. Maybe we should have some replacement for it, but dont think model needs to know how it failed. More that an image was supposed to be there?
| fallbackMimeType?: string | ||
| ) => Promise<string | null>; | ||
|
|
||
| export function createBlobToDataUrlHydrator( |
There was a problem hiding this comment.
should we add an explicit garbage collector for this as well as all other things in AgentContext like this?
destroy() { // Explicitly clean up resources this.ctx.mcpManager?.destroy?.(); this.ctx.contextResolver = undefined; this.ctx.credentialStuffer = undefined; this.ctx.credentialStoreRegistry = undefined; this.ctx.hydrateBlobToDataUrl = undefined; // Clear the hydrator & its cache this.ctx.functionToolRelationshipIdByName.clear(); this.ctx = null; }
There was a problem hiding this comment.
Good idea. Right now the Agent is created per-request and the hydrator + its cache are released by GC when the Agent is collected, so there's no leak in the current architecture. But adding explicit cleanup in a destroy() method is a defensive best practice — it makes the lifecycle explicit, prevents surprises if the Agent lifecycle ever changes, and helps with debugging memory. The snippet you outlined looks right. Setting ctx.hydrateBlobToDataUrl = undefined would release the closure and its captured Map.
|
|
||
| // Remove current message if it matches the last message (same as legacy) | ||
| let messagesToFormat = conversationHistory; | ||
| if (currentMessage && conversationHistory.length > 0) { |
There was a problem hiding this comment.
thank you for simplifying this!
| currentCompressor: MidGenerationCompressor | null; | ||
| functionToolRelationshipIdByName: Map<string, string>; | ||
| taskDenialRedirects: Array<{ toolName: string; toolCallId: string; reason: string }>; | ||
| /** When set, conversation history image parts with blob URIs are hydrated to data URLs (e.g. when the model supports images). */ |
There was a problem hiding this comment.
we probably dont need the comment here
There was a problem hiding this comment.
Agreed — the comment restates what the type name already conveys. Safe to remove.
| currentCompressor: MidGenerationCompressor | null; | ||
| functionToolRelationshipIdByName: Map<string, string>; | ||
| taskDenialRedirects: Array<{ toolName: string; toolCallId: string; reason: string }>; | ||
| /** When set, conversation history image parts with blob URIs are hydrated to data URLs (e.g. when the model supports images). */ |
There was a problem hiding this comment.
Also where is the check for models supporting images?
There was a problem hiding this comment.
Looking at the code, there isn't one — ctx.hydrateBlobToDataUrl is set unconditionally in Agent.ts:114 via createBlobToDataUrlHydrator(), so hydration runs for every agent regardless of whether the model supports images. Today that's probably fine if all models in use do support images, but if a text-only model is ever configured, it would receive data-URL image parts it can't process. Worth gating on a model capability check (or at least a config flag) before this ships more broadly.
| if (conversationHistoryWithFileData?.length) { | ||
| conversationHistoryWithFileData.forEach((msg) => { | ||
| const content = (msg.content?.parts ?? []).flatMap((part) => { | ||
| if (part.kind !== 'file' || typeof part.data !== 'string') { | ||
| return []; | ||
| } | ||
|
|
||
| return [ | ||
| { | ||
| type: 'image' as const, | ||
| image: part.data, | ||
| }, | ||
| ]; | ||
| }); | ||
|
|
||
| if (content?.length && content.length > 0) { | ||
| messages.push({ role: 'user', content }); | ||
| } | ||
| }); |
|
Responded to all 7 review comment threads from @tim-inkeep with thoughts and recommendations. Here's a summary: Replies posted
Suggested next steps for the PROrdered by priority:
Items 1–3 are straightforward changes for this PR. Items 4–6 could be follow-up work depending on scope preference. |
3 participants
No description provided.