feat(run): enable inline text document attachments via chat APIs#2821
feat(run): enable inline text document attachments via chat APIs#2821mike-inkeep merged 11 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 012f81d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
TL;DR — Adds inline base64 text document attachments ( Key changes
Summary ｜ 27 files ｜ 11 commits ｜ base: Shared MIME constants and extension maps
The allowlist set remains the single source of truth consumed by validation, security, and generation code paths. The regex covers both Request schema expansion with inline-only enforcement
The OpenAI-style path gains text document support through the expanded union. The Vercel path uses Text document attachment utilities
Security pipeline integration
Text formats lack magic bytes, so MIME allowlist + text-safety validation replaces byte-signature sniffing. The MIME type is now extracted before size validation (reordered from the previous flow).
Generation-time replay with split error handling
On failure, both paths produce an
Conversation history replay
This ensures persisted text attachments are included when conversation history is compressed or formatted for context injection — not silently dropped. Tracing timestamp format fix
A drive-by fix in both Documentation and design spec
A design spec at Test coverage
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured PR — the ingestion → blob persistence → replay pipeline is clean and the spec is thorough. Two items need attention before merge: (1) formatMessagesAsConversationHistory now silently drops artifact-ref reconstruction that reconstructMessageText provided, which is a behavioral regression for conversation history formatting in the compression path; (2) the docs understate the 256 KB limit scope (says "Plain text and Markdown" but it applies to all five text document types).
| } | ||
|
|
||
| return `${roleLabel}: """${reconstructMessageText(msg)}"""`; | ||
| return `${roleLabel}: """${await buildConversationMessageText(msg)}"""`; |
There was a problem hiding this comment.
formatMessagesAsConversationHistory previously called reconstructMessageText, which concatenated type === 'text' parts and converted type === 'data' artifact parts into <artifact:ref> tags. The replacement buildConversationMessageText only uses msg.content.text as the base and appends file attachment blocks — it drops text-part concatenation and artifact-ref reconstruction entirely.
This means conversation history produced via the compression path (getConversationHistoryWithCompression) will lose artifact-ref tags and any multi-part text assembly. Either buildConversationMessageText should fall through to reconstructMessageText for its base text, or reconstructMessageText should be called first and the result used as baseText.
|
|
||
| if ('bytes' in file && file.bytes) { | ||
| const normalized = await normalizeInlineFileBytes(file); | ||
| content = decodeTextDocumentBytes(normalized.data); |
There was a problem hiding this comment.
normalizeInlineFileBytes already calls decodeTextDocumentBytes(data) internally (in sniffAllowedInlineFileMimeType) to validate that the bytes are valid UTF-8 text without disallowed control characters. That decoded string is discarded, and then decodeTextDocumentBytes(normalized.data) decodes and validates the same bytes a second time. The double-decode is functionally correct but wasteful — consider having normalizeInlineFileBytes return the already-decoded string for text documents, or just decode once here.
|
|
||
| if (isTextDocumentMimeType(requestedMimeType)) { | ||
| if (!requestedMimeType) { | ||
| throw new BlockedInlineUnsupportedFileBytesError('unknown'); |
There was a problem hiding this comment.
Dead code: if isTextDocumentMimeType(requestedMimeType) returned true on line 97, requestedMimeType is guaranteed to be a non-empty string (since isTextDocumentMimeType(undefined) normalizes to '' which is not in the allowlist). This if (!requestedMimeType) branch is unreachable.
| - **Maximum size**: each inline file payload is limited to **10 MB** | ||
| - **Maximum size**: | ||
| - Images and PDFs: each inline file payload is limited to **10 MB** | ||
| - Plain text and Markdown: each inline file payload is limited to **256 KB** |
There was a problem hiding this comment.
This says the 256 KB limit applies to "Plain text and Markdown" but the code applies TEXT_DOCUMENT_MAX_BYTES to all five text document types (text/plain, text/markdown, text/html, text/csv, text/x-log). The docs should say "Text documents" or list all five types.
| - Plain text and Markdown: each inline file payload is limited to **256 KB** | |
| - Plain text, Markdown, HTML, CSV, and log files: each inline file payload is limited to **256 KB** |
| if (!isTextDocumentMimeType(mimeType) || !isBlobUri(part.data)) { | ||
| return ''; | ||
| } | ||
| if (!mimeType) { |
There was a problem hiding this comment.
Same dead-code pattern as in file-content-security.ts: if isTextDocumentMimeType(mimeType) is true, mimeType is guaranteed to be a non-empty string. The if (!mimeType) on this line is unreachable.
![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) 🟠⚠️
Inline Comments:
- 🟠 Major:
chat-api.mdx:140Documentation incorrectly scopes 256KB limit to only "Plain text and Markdown" instead of all five text document types - 🟠 Major:
chat.ts:94Missing test coverage forVercelFilePartSchemasuperRefine that rejects remote URLs for text MIME types (security constraint) - 🟠 Major:
text-document-attachments.ts:86Missing dedicated unit tests fordecodeTextDocumentBytesvalidation logic (control chars, line ending normalization, error types)
🟡 Minor (2) 🟡
🟡 1) text-document-attachments.ts:6 Size limit constant location inconsistency
Issue: TEXT_DOCUMENT_MAX_BYTES is defined in a utils module rather than alongside peer constant MAX_FILE_BYTES in file-security-constants.ts.
Why: Related file size limits should be co-located for discoverability and maintenance.
Fix: Consider moving TEXT_DOCUMENT_MAX_BYTES to file-security-constants.ts alongside MAX_FILE_BYTES.
Refs: file-security-constants.ts:3
🟡 2) conversations.ts + conversation-history.ts Duplicate blob resolution logic
Issue: Both buildConversationMessageText in conversations.ts and buildTextAttachmentPart in conversation-history.ts implement similar logic to resolve text attachments from blob storage and convert them to XML blocks.
Why: This duplication could lead to drift if one is updated without the other.
Fix: Consider extracting the shared blob-download-and-decode logic to the text-document-attachments.ts utility module as a single resolveTextAttachmentContent function.
Refs: conversation-history.ts:131-133, conversations.ts:947-949
💭 Consider (3) 💭
💭 1) text-document-attachments.ts:97 XML content escaping
Issue: buildTextAttachmentBlock doesn't escape XML-sensitive characters in content. User text containing </attached_file> could break the block structure.
Why: Primarily affects prompt semantics rather than security, but could cause unexpected model behavior.
Fix: Document as known limitation, or consider using CDATA sections for content. Note: filename/mediaType are already safely escaped via JSON.stringify().
💭 2) conversation-history.ts:129 Redundant double-decoding
Issue: buildTextAttachmentPart calls normalizeInlineFileBytes (which validates via decodeTextDocumentBytes) then immediately calls decodeTextDocumentBytes again.
Why: Performs duplicate work. Not incorrect, but inefficient.
Fix: Could refactor to have normalizeInlineFileBytes return decoded string for text MIME types.
💭 3) text-document-attachments.ts:29-52 Error class location
Issue: Text document error classes could be co-located with FileSecurityError hierarchy in file-security-errors.ts since they serve the same security validation purpose.
Why: Would improve discoverability.
Fix: Consider moving to file-security-errors.ts as subclasses of FileSecurityError, or document the intentional separation.
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-designed feature with solid security controls (256KB limit, UTF-8 validation, inline-only restriction, control character filtering). The implementation correctly follows the SPEC.md design decisions. The security reviewer found no IAM/auth concerns — the inline-only constraint for text documents effectively prevents SSRF, and tenant isolation is properly maintained through the data access layer.
The main gaps are in test coverage (3 Major findings) and a documentation accuracy issue. These are straightforward to address and don't block the feature's correctness. The code quality is good, patterns are mostly consistent with the codebase, and the XML-tagged injection approach is a pragmatic choice for cross-provider compatibility.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
message-parts.ts:121 |
Hardcoded mimeType: 'application/pdf' causing text documents to be mislabeled |
Invalid — Verified that buildFilePart extracts MIME type from data URI at line 69 (mimeType: parsed.mimeType), not from the passed option. The hardcoded PDF value is only used for HTTP URLs (line 91), not data URIs. |
text-document-attachments.ts:32 |
Error class uses new.target.name vs explicit string pattern |
Informational only — The pattern correctly follows FileSecurityError which is the appropriate peer for file security errors. |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
2 | 0 | 2 | 0 | 0 | 0 | 0 |
pr-review-tests |
7 | 0 | 0 | 0 | 2 | 0 | 5 |
pr-review-docs |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-consistency |
5 | 2 | 1 | 0 | 0 | 0 | 2 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 15 | 2 | 3 | 0 | 3 | 0 | 7 |
Note: Security reviewer returned 0 findings — confirmed solid security design with proper input validation, tenant isolation, and SSRF prevention via inline-only constraint.
| - **Maximum size**: each inline file payload is limited to **10 MB** | ||
| - **Maximum size**: | ||
| - Images and PDFs: each inline file payload is limited to **10 MB** | ||
| - Plain text and Markdown: each inline file payload is limited to **256 KB** |
There was a problem hiding this comment.
🟠 MAJOR: Inaccurate size limit scope in documentation
Issue: The docs state "Plain text and Markdown: each inline file payload is limited to 256 KB" but the implementation applies the 256 KB limit to all five text document types (text/plain, text/markdown, text/html, text/csv, text/x-log).
Why: Users uploading HTML, CSV, or log files over 256 KB will receive unexpected rejection errors since the docs imply only plain text and markdown have this restriction.
Fix:
| - Plain text and Markdown: each inline file payload is limited to **256 KB** | |
| - Images and PDFs: each inline file payload is limited to **10 MB** | |
| - Text documents (plain text, markdown, HTML, CSV, log): each inline file payload is limited to **256 KB** |
Refs:
| path: ['url'], | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing test coverage for remote URL rejection validation
Issue: The superRefine validation that rejects remote URLs for text document MIME types (a security constraint) is not tested. No test confirms that https:// URLs with text MIME types are rejected with the specific error message.
Why: If this validation were removed or broken, the API would accept remote text URLs, expanding the attack surface for SSRF and contradicting the design constraint of inline-only text attachments. This is a security-adjacent constraint that should have explicit test coverage.
Fix: Add a test to dataChat.test.ts or create schema validation tests:
it('should reject Vercel file part with remote URL for text/plain MIME type', async () => {
const response = await makeRequest('POST', '/run/api/chat', {
messages: [{
role: 'user',
content: 'test',
parts: [{
type: 'file',
url: 'https://example.com/notes.txt',
mediaType: 'text/plain'
}]
}]
});
expect(response.status).toBe(400);
// Verify error mentions inline base64 data URIs requirement
});Refs:
| throw new TextDocumentControlCharacterError(); | ||
| } | ||
|
|
||
| return decoded.replace(/\r\n?/g, '\n'); |
There was a problem hiding this comment.
🟠 MAJOR: Missing dedicated unit tests for text document validation logic
Issue: The decodeTextDocumentBytes function has no direct unit test coverage for its core validation logic. While file-content-security.test.ts exercises it indirectly via normalizeInlineFileBytes, this misses testing:
- The control character rejection logic in
hasDisallowedControlCharactersfor edge cases like NUL bytes (0x00), form feed (0x0C), or DEL (0x7F) - The line ending normalization (
\r\n?to\n) - The specific error types thrown (
InvalidUtf8TextDocumentErrorvsTextDocumentControlCharacterError)
Why: A regression in the UTF-8 decoding or control character filtering could allow binary data or malicious content to be injected into model prompts. This is security-adjacent validation.
Fix: Add a dedicated test file text-document-attachments.test.ts with tests:
describe('decodeTextDocumentBytes', () => {
it('throws InvalidUtf8TextDocumentError for invalid UTF-8 sequences', () => {
expect(() => decodeTextDocumentBytes(Buffer.from([0xC0, 0x80]))).toThrow(InvalidUtf8TextDocumentError);
});
it('throws TextDocumentControlCharacterError for NUL byte', () => {
expect(() => decodeTextDocumentBytes(Buffer.from('hello\x00world'))).toThrow(TextDocumentControlCharacterError);
});
it('throws TextDocumentControlCharacterError for DEL (0x7F)', () => {
expect(() => decodeTextDocumentBytes(Buffer.from('test\x7Fdata'))).toThrow(TextDocumentControlCharacterError);
});
it('normalizes CRLF to LF', () => {
const result = decodeTextDocumentBytes(Buffer.from('line1\r\nline2\rline3'));
expect(result).toBe('line1\nline2\nline3');
});
it('preserves allowed control chars (tab, newline)', () => {
const result = decodeTextDocumentBytes(Buffer.from('col1\tcol2\nrow2'));
expect(result).toBe('col1\tcol2\nrow2');
});
});Refs:
This PR adds inline text document attachments to the run chat APIs for `text/plain`, `text/markdown`, `text/html`, `text/csv`, and `text/x-log`. Accepted text attachments are validated as UTF-8 text, persisted as blob-backed file parts instead of raw inline payloads, and replayed into model input as XML-tagged `<attached_file ...>` blocks on both the initial turn and follow-up turns through conversation history. PDF URL support remains unchanged; remote URLs are still limited to PDFs. - Text document inputs are inline-only. PDFs continue to support both inline base64 and remote URLs, but text documents remain restricted to inline `data:` payloads to avoid widening the remote fetch surface. - Text attachments are normalized into XML-tagged text blocks for model input instead of provider-specific file parts. This keeps behavior consistent across providers and ensures persisted history replay matches first-turn prompt injection. - `text/html` is treated as raw HTML source, not rendered page text. The API does not sanitize or convert HTML before injection. - MIME handling is still driven by the submitter-provided media type. The server does not currently infer text attachment type from filename extension. No manual QA performed. Consider running `/qa` to generate and execute a test plan. - Add server-side MIME fallback from filename extension for clients that upload text-like files with missing or underspecified media types. - Consider whether additional text-like formats such as JSON or XML should follow the same inline-text attachment path.
mike-inkeep
force-pushed
the
feat/txt_attachments
branch
from
c6274e5 to
3aff3eb
Compare
Ito Test Report ❌18 test cases ran. 1 failed, 17 passed. Overall, 17 of 18 test cases passed, confirming expected behavior for inline attachment acceptance (including mixed/image+PDF+text payloads), rejection of invalid inputs (remote text URLs, unsupported MIME types, non-http schemes, oversize/binary-control text payloads), auth boundary enforcement, burst/concurrency and cross-route conversation stability, mobile-path execution, and documentation correctness for supported text MIME types and inline-only requirements. ❌ Failed (1)
🟠 Non-stream /run/api/chat can return empty assistant content on execution error
Relevant code:
const captured = bufferingHelper.getCapturedResponse();
return c.json({
choices: [
{
message: {
role: 'assistant',
content: captured.hasError ? captured.errorMessage : captured.text,
},
finish_reason: result.success && !captured.hasError ? 'stop' : 'error',
},
],
});
// Stream error operation
// Send error operation for execution exception
await sseHelper.writeOperation(
errorOp(`Execution error: ${errorMessage}`, currentAgentId || 'system')
);
async writeError(error: string | ErrorEvent): Promise<void> {
this.hasError = true;
this.errorMessage = typeof error === 'string' ? error : error.message;
}✅ Passed (17)
Commit: Tell us how we did: Give Ito Feedback |
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
🟠⚠️ Major (3) 🟠⚠️
Inline Comments:
- 🟠 Major:
conversation-history.ts:136Missing error handling for blob download failure in buildTextAttachmentPart - 🟠 Major:
text-document-attachments.test.ts:29Missing test coverage forbuildTextAttachmentBlock()function - 🟠 Major:
conversation-history.test.ts:37Missing test for blob-backed text attachment replay path
🟡 Minor (1) 🟡
🟡 1) text-document-attachments.ts:35-48 Missing test coverage for getDefaultTextDocumentFilename()
Issue: The MIME type → default filename mapping function has no direct test coverage.
Why: A typo in the switch statement (e.g., case 'text/markdwon':) would cause incorrect file extensions, affecting model behavior.
Fix: Add tests for all five MIME types plus unknown/charset suffix handling.
Refs: getDefaultTextDocumentFilename
💭 Consider (2) 💭
💭 1) text-document-attachments.ts:76 XML content injection semantics
Issue: The buildTextAttachmentBlock function injects user-provided content directly into the XML-tagged block without escaping. Content containing </attached_file> could theoretically affect model interpretation of the block boundaries.
Why: This is a prompt design consideration, not a traditional security vulnerability — the model receives raw text, not parsed XML. Whether this affects behavior depends on the model.
Fix: Document this as a known limitation in the SPEC.md or consider using CDATA sections if prompt semantics become an issue in practice.
💭 2) conversation-history.ts:132-133 Double-decode inefficiency (previously raised)
Issue: normalizeInlineFileBytes validates via decodeTextDocumentBytes, then buildTextAttachmentPart calls decodeTextDocumentBytes again.
Why: Functionally correct but wastes CPU for 256KB files.
Fix: Refactor to return decoded text from validation path, or accept as known minor overhead.
🕐 Pending Recommendations (1)
- 🟡
file-content-security.ts:101-102Dead code:if (!requestedMimeType)branch is unreachable afterisTextDocumentMimeType()returns true (raised by pullfrog)
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-designed feature with solid security controls (256KB limit, UTF-8 validation, inline-only restriction, control character filtering). The implementation correctly follows the SPEC.md design decisions, and the prior review feedback about documentation accuracy has been addressed. The main gaps are in error handling resilience (blob download failures) and test coverage (buildTextAttachmentBlock, blob replay path). These are straightforward to address and don't block the feature's correctness.
The XML-tagged injection approach is a pragmatic choice for cross-provider compatibility. The security controls are appropriate — the inline-only constraint prevents SSRF, and tenant isolation is maintained through the data access layer.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
text-document-attachments.ts:10-17 |
Tab/newline characters incorrectly blocked as control chars | Invalid — Code correctly allows 0x09 (tab) and 0x0A (LF). The ranges 0x0-0x8 and 0xe-0x1f intentionally exclude these. |
text-document-attachments.ts:22-23 |
Undefined codePoint handling fragile | Invalid — The || short-circuit correctly returns true before calling the range check function. This is idiomatic JS. |
chat.ts:84-94 |
Missing test for Vercel schema rejection | Already addressed — Test exists at dataChat.test.ts:370-395. |
text-document-attachments.test.ts |
Missing unit tests for decodeTextDocumentBytes | Already addressed — Tests exist covering CRLF normalization, invalid UTF-8, and control characters. |
text-document-attachments.ts:66-79 |
XML injection vulnerability (CRITICAL) | Downgraded to Consider — This is not traditional XML injection. The model receives raw text, not parsed XML. The concern is about prompt semantics, not security exploitation. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
5 | 0 | 1 | 0 | 1 | 0 | 3 |
pr-review-tests |
7 | 1 | 0 | 0 | 2 | 0 | 4 |
pr-review-appsec |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 13 | 1 | 2 | 0 | 3 | 0 | 7 |
Note: Docs reviewer confirmed documentation is accurate after prior feedback was addressed. Many test coverage findings were validated but some overlapped with prior reviews or were already addressed.
| content = decodeTextDocumentBytes(normalized.data); | ||
| } else if ('uri' in file && file.uri && isBlobUri(file.uri)) { | ||
| const downloaded = await getBlobStorageProvider().download(fromBlobUri(file.uri)); | ||
| content = decodeTextDocumentBytes(downloaded.data); |
There was a problem hiding this comment.
🟠 MAJOR: Missing error handling for blob download failure
Issue: If getBlobStorageProvider().download() throws (network error, blob not found, permission issue), the error propagates uncaught and crashes the generation stream.
Why: When a blob is deleted/expired but the conversation message still references it, users will see "Failed to process chat completion" instead of gracefully degrading. This is a runtime reliability concern for conversation history replay.
Fix: Wrap the blob download in a try-catch:
} else if ('uri' in file && file.uri && isBlobUri(file.uri)) {
try {
const downloaded = await getBlobStorageProvider().download(fromBlobUri(file.uri));
content = decodeTextDocumentBytes(downloaded.data);
} catch (err) {
logger.warn({ err, uri: file.uri, mimeType }, 'Failed to download text attachment from blob storage');
return {
type: 'text',
text: buildTextAttachmentBlock({
mimeType,
content: '[Attachment unavailable]',
filename
}),
};
}
}Refs:
|
|
||
| expect(() => decodeTextDocumentBytes(data)).toThrow(TextDocumentControlCharacterError); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing test coverage for buildTextAttachmentBlock()
Issue: The buildTextAttachmentBlock() function (lines 66-79 in text-document-attachments.ts) generates the XML template injected into model prompts, but has no direct test coverage.
Why: This function handles user-controlled content (filename, content). While JSON.stringify() is used for filename/MIME type escaping, there are no tests verifying: (1) filename escaping for quotes/special chars, (2) default filename fallback when undefined, (3) content with XML-like sequences. A future refactor could break escaping without detection.
Fix: Add test coverage:
describe('buildTextAttachmentBlock', () => {
it('escapes filenames with quotes via JSON.stringify', () => {
const result = buildTextAttachmentBlock({
mimeType: 'text/plain',
content: 'hello',
filename: 'file"with"quotes.txt',
});
expect(result).toContain('filename="file\\"with\\"quotes.txt"');
});
it('uses default filename when undefined', () => {
const result = buildTextAttachmentBlock({
mimeType: 'text/markdown',
content: '# Title',
});
expect(result).toContain('filename="unnamed.md"');
});
it('preserves content verbatim including XML-like sequences', () => {
const result = buildTextAttachmentBlock({
mimeType: 'text/html',
content: '<h1>Title</h1>',
filename: 'page.html',
});
expect(result).toContain('<h1>Title</h1>');
});
});Refs:
| ].join('\n'), | ||
| }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing test for blob-backed text attachment replay
Issue: This test only covers the inline bytes path (file.bytes present). The blob URI download path (file.uri with blob:// scheme) at lines 134-136 of conversation-history.ts has no test coverage.
Why: If the blob download integration is broken (wrong URI parsing, missing await, mock not configured), text attachments will fail during conversation history replay. Users would see errors like "Blob not found" on subsequent turns after uploading text files.
Fix: Add a test for blob-backed attachments:
it('downloads and injects blob-backed text attachments', async () => {
vi.spyOn(getBlobStorageProvider(), 'download').mockResolvedValue({
data: Buffer.from('# Title\n\nHello from blob', 'utf8'),
mimeType: 'text/markdown',
});
const content = await buildUserMessageContent('Summarize this', [
{
kind: 'file',
file: {
uri: 'blob://v1/t_tenant/media/p_proj/conv/c_conv/m_msg/sha256-abc123',
mimeType: 'text/markdown',
},
metadata: { filename: 'notes.md' },
},
]);
expect(getBlobStorageProvider().download).toHaveBeenCalledWith(
'v1/t_tenant/media/p_proj/conv/c_conv/m_msg/sha256-abc123'
);
expect(content[1]).toMatchObject({
type: 'text',
text: expect.stringContaining('# Title'),
});
});Refs:
Ito Test Report ✅8 test cases ran. 8 passed. The unified test run passed cleanly with 8 executed test cases, 8 passes, and 0 failures, while additional code-first verification produced no extra reportable coverage or defects. Key findings were that inline text attachments streamed successfully across both OpenAI-style and Vercel-style routes (including text/plain, text/markdown, text/csv, and text/x-log), the 256 KB boundary was accepted, malformed base64 and remote text-document URLs (even with uppercase/parameterized media types) were correctly rejected with HTTP 400, and a file-only message was handled safely with a controlled non-5xx response (404 in this run). ✅ Passed (8)
Commit: Tell us how we did: Give Ito Feedback |
Ito Test Report ❌6 test cases ran. 2 failed, 4 passed. Overall, the unified verification run produced 6 verifiable test cases with 4 passes and 2 high-severity failures, so the release is not clean due to confirmed production attachment-validation defects. The most critical findings were that /run/api/chat returns HTTP 200 while silently dropping invalid text attachments (including >256 KB payloads) and inconsistently accepts malformed base64 that /run/v1/chat/completions correctly rejects, while other checks passed for MIME normalization/remote text URL rejection, missing filename tolerance, concurrent duplicate-submit stability, and updated Chat API documentation constraints. ❌ Failed (2)
|
| Category | Summary | Screenshot |
|---|---|---|
| Doc | Chat API docs include required text MIME types, inline-only guidance, and size limits. |  |
| Edge | Text/Plain; charset=UTF-8 inline data URI is accepted and remote text URL is rejected with a 400 inline/data-uri validation path. |
 |
| Edge | Missing filename metadata is accepted and text attachment processing continues on both endpoint shapes. |  |
| Rapid | Re-executed concurrent duplicate submits remained stable; all five requests returned 200 with x-vercel-ai-data-stream: v2. |
 |
Commit: ed38ae7
Tell us how we did: Give Ito Feedback
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Review Scope: Delta review — changes since commit a338dbcc18d8 (last automated review that gave ✅ APPROVE)
Delta Changes Analyzed
The delta consists of two meaningful changes:
| Commit | Change | Assessment |
|---|---|---|
ed38ae7f0 |
Add application/json text file attachments |
✅ Follows established patterns exactly |
74253ecff |
Fix tracing timestamp bug (Date.now() → new Date().toISOString()) |
✅ Unrelated improvement, correct fix |
application/json Support Verification
The JSON MIME type addition follows the exact same pattern as the other 5 text document types across all surfaces:
| Surface | Status | Evidence |
|---|---|---|
| MIME allowlist | ✅ | Added to ALLOWED_TEXT_DOCUMENT_MIME_TYPES in allowed-file-formats.ts |
| Data URI regex | ✅ | DATA_URI_TEXT_BASE64_REGEX updated to include application/json |
| Extension mappings | ✅ | Added 'application/json': 'json' and json: 'application/json' |
| Default filename | ✅ | Added case 'application/json': return 'unnamed.json' |
| Size limit (256 KB) | ✅ | Inherited via isTextDocumentMimeType() check |
| Zod schema validation | ✅ | TextDocumentDataUriSchema regex covers application/json |
| Remote URL rejection | ✅ | VercelFilePartSchema.superRefine() applies to all text MIME types |
| Route tests | ✅ | Added tests in chat.test.ts and dataChat.test.ts |
| Unit tests | ✅ | Added to text-document-attachments.test.ts, file-content-security.test.ts, file-upload.test.ts |
| Integration tests | ✅ | Added "injects inline JSON attachments as XML attachment blocks" test |
| Documentation | ✅ | Updated chat-api.mdx with JSON examples and in all relevant lists |
Prior Feedback Status
All findings from previous reviews have been addressed. The prior automated review (2026-03-26) gave ✅ APPROVE with "All prior findings addressed."
✅ APPROVE
Summary: The delta is minimal and well-executed. Adding application/json support follows the established patterns exactly — MIME allowlist, regex, mappings, tests, and docs are all correctly updated. The tracing timestamp fix is a clean improvement. No new issues introduced. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (manual delta review) |
2 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 2 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review focused on commits since a338dbcc18d8. Both changes (JSON support, timestamp fix) are clean and follow existing patterns.
Ito Test Report ❌8 test cases ran. 2 failed, 6 passed. Overall, the unified run executed 8 test cases with 6 passes and 2 confirmed failures, showing that most attachment and streaming behaviors worked as expected but the release is not fully clean. Key findings were a high-severity regression introduced by this PR where binary/control-character text attachments are incorrectly accepted with HTTP 200 on both chat routes, and a medium-severity pre-existing bug where context/header validation errors are misclassified as HTTP 500 instead of client 4xx, while other checks passed including inline text MIME acceptance (with v2 stream header), mediaType/url mismatch rejection (400), docs/mobile contract visibility, and stable 10-way concurrent submissions without 5xx. ❌ Failed (2)
🟠 Context validation middleware returns 500 instead of client validation error
Relevant code:
if (!validationResult.valid) {
logger.warn(
{
tenantId,
agentId,
errors: validationResult.errors,
},
'Headers validation failed'
);
const errorMessage = `Invalid headers: ${validationResult.errors.map((e) => `${e.field}: ${e.message}`).join(', ')}`;
throw createApiError({
code: 'bad_request',
message: errorMessage,
});
}
} catch (error) {
logger.error(
{
error: error instanceof Error ? error.message : 'Unknown error',
},
'Context validation middleware error'
);
throw createApiError({
code: 'internal_server_error',
message: 'Context validation failed',
});
}
return {
valid: false,
errors: [
{
field: 'validation',
message: 'Context validation failed due to internal error',
},
],
};
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | mediaType/url mismatch payloads were rejected with HTTP 400 as expected. |  |
| Adversarial | 10 parallel attachment submissions completed without hangs or 5xx, showing stable concurrent request handling in the exercised path. |  |
| Edge | Confirmed this was not a product defect; source checks and targeted re-execution support valid inline text/html handling and stream behavior. |
 |
| Happy-path | OpenAI chat accepted all tested inline text MIME variants with HTTP 200 SSE responses. |  |
| Happy-path | Re-execution passed: /run/api/chat accepted inline text file parts with 200 SSE and v2 stream header. |  |
| Happy-path | Docs contract text and mobile parity validated on local docs app. |  |
Commit: 02c7740
Tell us how we did: Give Ito Feedback
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Review Scope: Delta review — changes since commit 02c774018152 (last automated review that gave ✅ APPROVE)
Delta Analysis
The delta consists of 1 commit (134d60929) titled "Tighten up route validation" that modifies 8 files with +269/-23 lines. This is a targeted fix that addresses the Ito QA failures where invalid text attachments were returning HTTP 200 instead of HTTP 400.
| Layer | Change | Assessment |
|---|---|---|
Routes (chat.ts, chatDataStream.ts) |
Catch FileSecurityError and convert to HTTP 400 |
✅ Correct — errors now surface as client validation failures |
File upload (file-upload.ts) |
Rethrow FileSecurityError instead of swallowing |
✅ Correct — security errors propagate up the call stack |
Upload helpers (file-upload-helpers.ts) |
Rethrow FileSecurityError instead of falling back to text-only |
✅ Correct — prevents silent data loss |
Route tests (chat.test.ts, dataChat.test.ts) |
Add rejection tests for oversized, malformed base64, and binary payloads | ✅ Comprehensive coverage for both routes |
Upload tests (file-upload.test.ts, file-upload-helpers.test.ts) |
Add rethrow behavior tests | ✅ Verifies error propagation works |
Why This Fix Is Correct
The prior behavior had a fail-open pattern where:
normalizeInlineFileBytescorrectly threwBlockedInlineFileExceedingErrororBlockedInlineUnsupportedFileBytesErroruploadPartsFilescaught all errors and logged + dropped the file partbuildPersistedMessageContentcaught all errors and fell back to text-only- Routes returned HTTP 200 with the attachment silently missing
The fix introduces fail-closed behavior for security errors:
uploadPartsFilesnow checksif (error instanceof FileSecurityError) throw error;before the generic catchbuildPersistedMessageContentdoes the same- Routes catch
FileSecurityErrorexplicitly and convert tobad_request(HTTP 400)
This matches the existing pattern for PdfUrlIngestionError and ensures validation failures are surfaced to clients.
Ito QA Findings Addressed
| Ito Finding | Status |
|---|---|
| ✅ Fixed — now returns HTTP 400 | |
/run/api/chat |
✅ Fixed — now returns HTTP 400 |
| ✅ Fixed — now returns HTTP 400 |
Prior Feedback Status
All findings from previous automated reviews were addressed in earlier commits. The last automated review (2026-03-27) gave ✅ APPROVE. This delta adds the final piece needed for the feature to be complete.
✅ APPROVE
Summary: Excellent targeted fix. The error propagation changes are minimal, correct, and well-tested. This closes the loop on the Ito QA failures without introducing any regressions. The PR is ready to merge. 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta review) |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta focused on error propagation fix. All changes follow established patterns and are correctly implemented.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Review Scope: Delta review — changes since commit 134d6092950a (last automated review that gave ✅ APPROVE)
Delta Analysis
The delta since the last automated review consists of 7 commits with 56 files changed. However, none of the delta changes affect the PR's feature scope:
| Change Type | Files | Assessment |
|---|---|---|
| Version bumps / changesets | 14 | ✅ Automated release machinery |
| CHANGELOG updates | 10 | ✅ Generated by changesets |
package.json updates |
12 | ✅ Version increments only |
| Unrelated features | 11 | ✅ Playground app seeding, eval fixes, docs improvements |
| Other docs/images | 9 | ✅ CLI pull docs, auth docs, conflict resolution images |
Overlap Check
Verified that none of the following PR feature files were modified in the delta:
text-document-attachments.ts— unchangedfile-content-security.ts— unchangedfile-upload.ts/file-upload-helpers.ts— unchangedconversation-history.ts— unchangedchat.ts/chatDataStream.ts— unchangedchat-api.mdx— unchangedallowed-file-formats.ts— unchanged- All test files — unchanged
Prior Review Status
| Reviewer | Verdict | Date |
|---|---|---|
| tim-inkeep (human) | ✅ APPROVED | 2026-03-26 |
| claude (automated) | ✅ APPROVE | 2026-03-27 |
All prior findings have been addressed. The feature implementation (inline text document attachments) is complete with:
- Comprehensive test coverage for both routes
- Security validation (256 KB limit, UTF-8, control chars, malformed base64)
- Error propagation (FileSecurityError → HTTP 400)
- Documentation updates
✅ APPROVE
Summary: The delta since the last review contains only merge commits from main (version bumps, unrelated features). No changes to the PR's feature implementation. The PR was already approved and remains ready to merge. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta review) |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta contained no feature-relevant changes — only merge commits from main branch.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs' |
|
📝 Documentation is already included in this PR — the Chat API docs have been updated to cover inline text document attachments. No additional docs changes needed. |
Ito Test Report ❌8 test cases ran. 1 failed, 7 passed. Overall, the unified run was largely successful with 7 of 8 executed test cases passing, including correct inline text attachment boundary/validation behavior (256KB accepted, oversize/malformed/control-character payloads rejected with 400), expected auth-boundary rejection without headers, stable abort-and-retry behavior, and successful cross-route conversation continuation. The most important finding was one high-severity reliability defect: parallel same-conversation requests to POST /run/v1/chat/completions can intermittently return HTTP 500 due to a pre-existing non-atomic check-then-insert race in conversation creation (createOrGetConversation). ❌ Failed (1)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Request without auth/context headers was rejected with a non-200 response (404), matching expected boundary behavior. |  |
| Edge | 256KB inline text attachment was accepted on /run/api/chat with HTTP 200 and stream start. |
 |
| Edge | 262145-byte inline text attachment was rejected with HTTP 400 size-limit validation. |  |
| Edge | Malformed base64 text data URI was rejected with HTTP 400 without crashing. |  |
| Edge | Control-character text payload was rejected with stable HTTP 400 security validation. |  |
| Flow | Verified mixed-route continuation for conv-mixed-1: OpenAI-route call streamed successfully, then Vercel-route follow-up streamed to completion without malformed history/errors after environment repair. |
 |
| Res | Abort-and-retry behavior is stable: after aborting first /run/api/chat stream, immediate retry with same conversationId succeeds (HTTP 200) without deadlock/stuck state. |
 |
Commit: 012f81d
Tell us how we did: Give Ito Feedback
2 participants
Summary
Extends the run chat APIs to accept inline base64 text document attachments (
text/plain,text/markdown,text/html,text/csv,text/x-log). Accepted attachments are validated as UTF-8, stored in blob storage as URI-backed file parts, and replayed into model input as XML-tagged<attached_file ...>blocks — on the initial turn and through conversation history replay. Remote URLs remain disallowed for all non-image document types.Key decisions
Inline-only, no remote URLs for text documents. PDF URLs are still accepted, but text document attachments are restricted to inline
data:payloads to avoid widening the remote-fetch attack surface. This was an explicit constraint from the start and is enforced at the schema validation layer.XML-tagged text blocks instead of provider-specific file parts. Text documents are injected as
<attached_file filename="..." media_type="...">XML blocks rather than as AI SDK file parts. This keeps injection behavior consistent across all model providers and ensures that the first-turn prompt and conversation-history replay produce identical prompts — no provider detection logic, no divergence.Blob-backed persistence only — no raw text in the DB. Decoded attachment content is never written into conversation message rows. The blob URI and MIME type are what's persisted; the decoded text is fetched transiently at generation time and injected into model context. This matches the existing image/PDF pattern.
256 KB decoded-byte cap per text attachment. Chosen to prevent runaway token usage while still accommodating typical log, CSV, and Markdown files. Whether this is tight enough for strict prompt-budget protection is an open question flagged in the spec.
text/htmlis raw source, not rendered text. The API injects HTML source without sanitizing, converting, or rendering it. This means models see raw tags, not page text. This is a known tradeoff — noted in the PR description and flagged as a follow-up.MIME type is submitter-controlled. The server does not infer text MIME type from filename extension. Clients must supply a correct
Content-Type/ media type. Server-side MIME fallback from extension is a deferred follow-up.Manual QA
No manual QA performed. Consider running
/qato generate and execute a test plan.Future considerations
.loguploaded asapplication/octet-stream).