ArtifactService binary sanitization and child artifacts#2745
ArtifactService binary sanitization and child artifacts#2745mike-inkeep wants to merge 6 commits intostack/artifact_binary_sanitizerfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: Medium
🟠⚠️ Major (3) 🟠⚠️
🟠 1) ArtifactService.ts:1007-1059 Partial batch failure leaves inconsistent state
Issue: The for-loop in createBinaryChildArtifacts iterates through binary parts and calls upsertLedgerArtifact for each one sequentially. If the database insert fails mid-way through the batch (e.g., on the 3rd of 5 parts), some child artifacts are persisted while others are not. The error propagates up, but the refs Map will be incomplete.
Why: Consider an artifact with 5 images: if child artifact creation succeeds for images 1-2 but fails on image 3, the error propagates and saveArtifact fails. However, child artifacts 1-2 remain in the database as orphaned records. If retried, deduplication may behave unexpectedly. The parent artifact's data structure would be inconsistent - some binary parts with artifactRef, others without.
Fix: Options to consider:
- Wrap in transaction for atomicity (if supported by the upsert function)
- Use
Promise.allto fail atomically if any fail:
const insertPromises = binaryParts.map(async (part) => {
const hash = this.extractContentHashFromBlobUri(part.data) || this.fallbackHash(part.data);
// ... build childArtifactId, check dedupe ...
await upsertLedgerArtifact(runDbClient)({ ... });
return { blobUri: part.data, ref: { artifactId, toolCallId } };
});
const results = await Promise.all(insertPromises);- Catch per-part errors and continue with partial success + explicit tracking
Refs:
🟠 2) ArtifactService.ts:906-911 Binary child artifacts only created for fullData, not summaryData
Issue: Binary child artifacts are created by traversing sanitizedData only. If artifact.summaryData contains blob-backed binary parts that are NOT present in artifact.data, those binaries will not have child artifacts created. The attachBinaryArtifactRefs call on summaryData will then fail to find refs for those parts, leaving them without artifactRef.
Why: If a caller provides different binary content in summaryData vs data (e.g., a thumbnail in summary, full image in data), the summary's thumbnail would have an orphaned blob URI with no child artifact record.
Fix: Consider whether summaryData can contain different binary parts than data. If yes, collect binary parts from both:
const binaryReferences = await this.createBinaryChildArtifacts({
parentArtifactId: artifact.artifactId,
parentArtifactType: artifact.type,
toolCallId: artifact.toolCallId,
value: sanitizedSummaryData ? [sanitizedData, sanitizedSummaryData] : sanitizedData,
});Or document that summaryData must be a subset of data for binary content.
Refs:
🟠 3) system Implicit contract change: artifact data now includes artifactRef for binary parts
Issue: The PR introduces an implicit data contract change. Artifact data structures (summaryData and fullData) will now contain artifactRef objects embedded within binary parts:
Before: { type: 'image', data: 'blob://...', mimeType: 'image/png' }
After: { type: 'image', data: 'blob://...', mimeType: 'image/png', artifactRef: { artifactId: 'bin_...', toolCallId: '...' } }
Why: This affects consumers retrieving artifacts via:
- Chat API (Vercel AI SDK Data Stream)
getContextArtifacts,getArtifactSummary,getArtifactFull- Streaming via
data-artifactevents - UI components rendering artifact data
While additive fields are generally backward-compatible, consumers doing strict validation or field enumeration may be affected.
Fix: Consider documenting this contract change. If artifactRef is intended to be a stable API for clients to resolve binary artifacts, add it to validation schemas and document the shape.
Refs:
Inline Comments:
- 🟠 Major:
ArtifactService.ts:997Silent fallback when context is missing - 🟠 Major:
ArtifactService.ts:1163-1165Regex requires literal.after hash
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
ArtifactService.ts:1148-1160Type guard returnstype: stringinstead of literal union
💭 Consider (1) 💭
💭 1) ArtifactService.ts:1044-1051 Child artifacts use metadata.parentArtifactId instead of existing derivedFrom column
Issue: The ledger_artifacts table already has a derivedFrom column designed for parent-child artifact relationships. This PR stores the relationship in metadata.parentArtifactId instead, creating two mechanisms for the same concept.
Why: The derivedFrom column is already wired into the DAL (maps from metadata.derivedFrom). Having two ways to express parent-child relationships may lead to inconsistent queries.
Fix: Consider using derivedFrom in metadata (or directly) for consistency:
metadata: {
derivedFrom: params.parentArtifactId, // Uses existing column
parentArtifactType: params.parentArtifactType,
// ...
}Refs:
Inline Comments:
- 💭 Consider:
ArtifactService.test.ts:1052Additional test coverage for edge cases
💡 APPROVE WITH SUGGESTIONS
Summary: This PR introduces a useful pattern for managing binary content in artifacts by extracting binaries into child artifacts with deduplicated storage. The main areas for improvement are: (1) partial batch failures can leave inconsistent state with orphaned child artifacts, (2) missing logging for silent fallbacks makes debugging difficult, and (3) the implicit contract change should be documented. The test coverage is a good start but could be expanded to cover edge cases. None of these are blockers, but addressing the error handling and logging would improve production reliability.
Discarded (9)
| Location | Issue | Reason Discarded |
|---|---|---|
ArtifactService.ts:1007 |
Sequential upserts for child artifacts | Intentional for deduplication logic; parallelization would break the dedupe Map accumulation |
ArtifactService.ts:1008-1014 |
Deduplication scope limited to tool call | Working as designed - per-tool-call isolation provides clearer provenance |
ArtifactService.ts:1021 |
Synthetic toolCallId pattern is new | The :binary suffix is actually good practice - distinguishes from real tool call IDs |
ArtifactService.ts:1030 |
Child artifact type naming uses hyphens | Consistent with existing test data showing binary-child type |
ArtifactService.ts:1037 |
blobUri vs data field name |
Internal storage structure, not a consistency issue |
ArtifactService.ts:989-1061 |
Extract named interfaces for types | Valid suggestion but INFO level - design preference |
ArtifactService.ts:1064-1096 |
Return type looser than guard | Follows naturally from type guard fix - INFO level |
ArtifactService.ts:1168-1170 |
fallbackHash uses hex encoding not SHA | Fallback path for edge cases, collision risk is minimal |
ArtifactService.ts:1172-1179 |
bin_ prefix is new naming pattern |
Acceptable new pattern - clearly distinguishes binary children |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-errors |
4 | 1 | 0 | 0 | 1 | 0 | 2 |
pr-review-standards |
3 | 1 | 0 | 0 | 1 | 0 | 1 |
pr-review-architecture |
5 | 1 | 1 | 0 | 0 | 0 | 3 |
pr-review-tests |
7 | 0 | 0 | 0 | 1 | 0 | 6 |
pr-review-types |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-consistency |
6 | 0 | 0 | 0 | 0 | 0 | 6 |
| Total | 28 | 3 | 1 | 0 | 4 | 0 | 20 |
| }): Promise<Map<string, { artifactId: string; toolCallId: string }>> { | ||
| if (!this.context.taskId || !this.context.contextId) { | ||
| return new Map(); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Silent fallback when context is missing
Issue: When taskId or contextId is missing, createBinaryChildArtifacts silently returns an empty Map with no logging. The caller has no indication that binary child artifact creation was skipped.
Why: This creates orphaned blob URIs in parent artifacts that have no corresponding child artifact records. Debugging this in production would require correlating blob storage contents with artifact records - difficult to diagnose without logs indicating the skip occurred.
Fix:
| } | |
| if (!this.context.taskId || !this.context.contextId) { | |
| logger.warn( | |
| { parentArtifactId: params.parentArtifactId, hasTaskId: !!this.context.taskId, hasContextId: !!this.context.contextId }, | |
| 'Skipping binary child artifact creation due to missing context' | |
| ); | |
| return new Map(); | |
| } |
Refs:
| private extractContentHashFromBlobUri(blobUri: string): string | null { | ||
| const match = blobUri.match(/sha256-([a-f0-9]{16,64})\./i); | ||
| return match?.[1] || null; |
There was a problem hiding this comment.
🟠 MAJOR: Regex requires literal . after hash - fragile for URIs without extensions
Issue: The regex /sha256-([a-f0-9]{16,64})\./i requires a literal . after the hash. Blob URIs without an extension (e.g., blob://v1/.../sha256-abc123 with no trailing .ext) will fail to match, causing fallback to hex-encoding the entire URI.
Why: This could cause deduplication failures if the same binary is referenced via URIs with and without extensions. The fallback hash would differ from the extracted hash.
Fix:
| private extractContentHashFromBlobUri(blobUri: string): string | null { | |
| const match = blobUri.match(/sha256-([a-f0-9]{16,64})\./i); | |
| return match?.[1] || null; | |
| private extractContentHashFromBlobUri(blobUri: string): string | null { | |
| const match = blobUri.match(/sha256-([a-f0-9]{16,64})(?:\.|$)/i); | |
| return match?.[1] || null; | |
| } |
Refs:
- buildStorageKey always adds extension - current code may be safe if all blob URIs have extensions, but defensive regex is safer
| private isBlobBackedBinaryPart( | ||
| value: unknown | ||
| ): value is { type: string; data: string; mimeType?: string } { | ||
| if (!value || typeof value !== 'object' || Array.isArray(value)) { | ||
| return false; | ||
| } | ||
|
|
||
| const maybePart = value as Record<string, unknown>; | ||
| return ( | ||
| (maybePart.type === 'image' || maybePart.type === 'file') && | ||
| typeof maybePart.data === 'string' && | ||
| isBlobUri(maybePart.data) | ||
| ); |
There was a problem hiding this comment.
🟡 Minor: Type guard returns type: string instead of literal union
Issue: The type guard validates only 'image' | 'file' for the type field but returns a predicate with type: string. This loses type information that could enable exhaustive checking downstream.
Why: Downstream code consuming the narrowed type cannot safely narrow further since TypeScript sees type: string instead of the literal union.
Fix:
| private isBlobBackedBinaryPart( | |
| value: unknown | |
| ): value is { type: string; data: string; mimeType?: string } { | |
| if (!value || typeof value !== 'object' || Array.isArray(value)) { | |
| return false; | |
| } | |
| const maybePart = value as Record<string, unknown>; | |
| return ( | |
| (maybePart.type === 'image' || maybePart.type === 'file') && | |
| typeof maybePart.data === 'string' && | |
| isBlobUri(maybePart.data) | |
| ); | |
| private isBlobBackedBinaryPart( | |
| value: unknown | |
| ): value is { type: 'image' | 'file'; data: string; mimeType?: string } { | |
| if (!value || typeof value !== 'object' || Array.isArray(value)) { | |
| return false; | |
| } | |
| const maybePart = value as Record<string, unknown>; | |
| return ( | |
| (maybePart.type === 'image' || maybePart.type === 'file') && | |
| typeof maybePart.data === 'string' && | |
| isBlobUri(maybePart.data) | |
| ); | |
| } |
Refs:
- InlineBinaryPart in artifact-binary-sanitizer.ts - uses
type: 'image' | 'file'pattern
| expect(upsertFn).toHaveBeenCalledTimes(2); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💭 Consider: Additional test coverage for edge cases
Issue: The new tests cover the happy path well but miss several edge cases:
- Missing context - No test for when
taskId/contextIdis missing (early return path) - No binary parts - No test confirming correct behavior when artifact has no blob-backed binaries
summaryDatapath - No test when bothdataandsummaryDataare providedtype: 'file'- All tests usetype: 'image'; thefiletype path is untested- Deeply nested parts - Tests only cover flat structures, not
{ outer: { inner: { images: [...] } } }
Why: These paths exist in the implementation and should have test coverage to prevent regressions.
Fix: Consider adding test cases for these scenarios. Example for missing context:
it('skips binary child artifact creation when taskId is missing', async () => {
const serviceWithoutTask = new ArtifactService({
...mockContext,
taskId: undefined,
});
// ... verify only parent artifact created
});Refs:
Ito Test Report ✅14 test cases ran. 14 passed. 🔍 Verification focused on artifact persistence, dedupe/normalization behavior, edge-case handling, and adversarial route protections. The included cases all met expected behavior with representative evidence timestamps and screenshots. ✅ Passed (14)
📋 View Recording |
This function is intentionally exported for use in artifact persistence (see PR #2745). The observability-only stripping path is `stripBinaryDataForObservability`. export async function sanitizeArtifactBinaryData()
…rvice_binary_refs
…ack, bulk child insert - Add ARTIFACT_SAVE_MAX_RETRIES (1) and ARTIFACT_SAVE_RETRY_DELAY_MS (3s) constants, distinct from ARTIFACT_GENERATION_MAX_RETRIES which covers LLM retries - recordEvent .catch handler now actively retries processArtifact once after delay; logs on exhaustion so setSpanWithError fires on the process_artifact span - Remove try/catch inline fallback in uploadInlinePart — blob upload errors now propagate out of sanitizeArtifactBinaryData instead of silently storing raw base64 - Replace per-row upsertLedgerArtifact loop in createBinaryChildArtifacts with a single bulkInsertLedgerArtifacts call (INSERT ... ON CONFLICT DO NOTHING) for single-statement atomicity - Fix ArtifactParser.typeSchema test mock missing getTracer (pre-existing breakage)
Add ARTIFACT_BINARY_CHILDREN span name and ARTIFACT_BINARY_CHILD_COUNT / ARTIFACT_BINARY_CHILD_IDS / ARTIFACT_BINARY_CHILD_HASHES span keys to otel-attributes. Parse and surface them in the traces timeline so binary child artifact count and IDs are visible when inspecting an artifact span.
|
TL;DR — Extends Key changes
Summary ｜ 14 files ｜ 6 commits ｜ base: Binary child artifact extraction
The extraction uses a two-pass recursive tree walk with
Fail-loud binary sanitizer with session-level retry
New execution limit constants
Binary child metadata in OTel and traces UI
|
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
RE-REVIEW: This review covers the delta since the prior review. Key improvements address the batch failure atomicity and error handling concerns.
✅ Prior Issues Addressed
| Prior Issue | Resolution |
|---|---|
| 🟠 Partial batch failure leaves inconsistent state | Fixed — Now uses bulkInsertLedgerArtifacts with onConflictDoNothing() for atomic batch insert (ledgerArtifacts.ts:241) |
💭 Uses metadata.parentArtifactId instead of derivedFrom |
Fixed — Now correctly uses derivedFrom in metadata (ArtifactService.ts:1080) |
| 🟠 Silent fallback masks upload failures | Fixed — artifact-binary-sanitizer.ts now throws on upload failure, enabling higher-level retry logic (artifact-binary-sanitizer.ts:54) |
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
AgentSession.ts:464Off-by-one semantics in retry logic naming
💭 Consider (1) 💭
💭 1) ArtifactService.ts:1206 Type guard returns type: string instead of literal union
Issue: The type guard isBlobBackedBinaryPart returns value is { type: string; ... } but the implementation checks for specific literals 'image' | 'file'.
Why: The loose type signature means TypeScript won't catch if code accidentally uses an invalid type value. While functionally correct, a stricter return type would provide better compile-time safety.
Fix: Update the return type to use the literal union:
private isBlobBackedBinaryPart(
value: unknown
): value is { type: 'image' | 'file'; data: string; mimeType?: string } {Refs:
🕐 Pending Recommendations (2)
- 🟠
ArtifactService.ts:906-911Binary child artifacts only created for fullData, not summaryData — acceptable if summaryData is always a subset of fullData - 🟠
ArtifactService.tsImplicit contract change withartifactRefinjection — consumers need to be aware of the new field
✅ APPROVE
Summary: The delta commits significantly improve reliability by switching to atomic bulk inserts and implementing active retry with explicit failure propagation. The prior review's major concerns about partial batch failures have been well addressed. The remaining items are minor (retry naming semantics) or pre-existing considerations from the prior review. Good work on the reliability improvements! 🎉
Discarded (0)
No findings discarded.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
4 | 0 | 1 | 0 | 1 | 2 | 0 |
| Total | 4 | 0 | 1 | 0 | 1 | 2 | 0 |
Note: Delta review — focused on changes since prior automated review. Sub-reviewers not dispatched due to small, focused delta scope.
| this.artifactProcessingErrors.set(artifactId, errorCount); | ||
|
|
||
| if (errorCount >= this.MAX_ARTIFACT_RETRIES) { | ||
| if (errorCount > this.MAX_SAVE_RETRIES) { |
There was a problem hiding this comment.
🟡 Minor: Off-by-one in retry logic
Issue: The condition errorCount > this.MAX_SAVE_RETRIES is checked after incrementing errorCount, so with MAX_SAVE_RETRIES = 1, this allows 2 attempts (initial + 1 retry) which is correct, but the naming/semantics are confusing.
Why: With MAX_SAVE_RETRIES = 1:
- First failure:
errorCount = 1, condition1 > 1is false → retry scheduled - Second failure:
errorCount = 2, condition2 > 1is true → give up
This is actually correct behavior (1 retry = 2 total attempts), but the variable name MAX_SAVE_RETRIES suggests "max number of retries" not "max error count before giving up". Consider renaming to MAX_SAVE_ATTEMPTS or adjusting the condition to errorCount >= this.MAX_SAVE_RETRIES.
Refs:
Ito Test Report ✅20 test cases ran. 20 passed. The unified QA run passed all 20 of 20 test cases with zero failures, confirming end-to-end correctness and stability of conversation trace artifact handling across API responses and timeline/detail-panel rendering. Key findings were that binary child metadata is correctly mapped and displayed (including count/IDs, array normalization, and duplicate-payload deduplication), safely omitted when absent, resilient under malformed input, large payloads, mobile and rapid interaction/refresh/cross-tab scenarios, protected against XSS/auth and tenant/project/path tampering leakage, and that SigNoz failure modes are surfaced with the expected 503 (unavailable), 501 (missing API key), and 429 (rate limit) responses without crashes or internal data exposure. ✅ Passed (20)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
No description provided.