Skip to content

Studio: import Open WebUI chat exports - #8643

Merged
danielhanchen merged 16 commits into
unslothai:mainfrom
oobabooga:worktree-openwebui-import
Aug 13, 2026
Merged

Studio: import Open WebUI chat exports#8643
danielhanchen merged 16 commits into
unslothai:mainfrom
oobabooga:worktree-openwebui-import

Conversation

@oobabooga

Copy link
Copy Markdown
Member

Problem

Studio rejects Open WebUI chat exports because they use .json, while chat import accepts only .jsonl, .ndjson, and .csv. Desktop imports are also limited to 64 MiB, but real exports can be hundreds of megabytes.

Open WebUI writes the entire export as one JSON array. Reading that file with file.text() creates a single string and can exceed V8's string limit, so accepting the extension alone would not support large exports.

Solution

  • Stream JSON arrays, JSONL, and NDJSON one record at a time instead of loading the whole file into a string.
  • Accept .json in the browser and desktop file pickers.
  • On desktop, return a scoped file token from the picker and read the selected file in 8 MiB byte ranges. The webview cannot use the token to read another path.
  • Save up to six threads concurrently and show import progress.
  • Skip malformed records and report read or write failures. Reject a file that ends mid-record because later records cannot be recovered safely.

Imported data

  • Preserve conversation branches and reopen the branch selected by history.currentId.
  • Convert modern Open WebUI tool output and legacy tool-call details into Studio messages.
  • Preserve reasoning, inline images, archived state, and message timestamps.
  • Recover usable chats with damaged history, including missing or cyclic parents, duplicate message IDs, dangling current IDs, and missing history objects.

Limits

  • Document attachments keep their names, but not server-extracted text, to avoid resending an entire document to the model on the next turn.
  • Exported /api/v1/files/... image URLs are not portable, so only inline image data is retained.
  • A chat containing only a failed turn is skipped because it has nothing to display.
  • CSV still uses the existing whole-file path and 64 MiB limit.
  • Existing JSONL, ShareGPT, and CSV behavior is preserved. Their import code was moved out of the prompt dialog for direct testing.

Validation

  • npm test (2,034 passed)
  • npm run typecheck
  • npm run build
  • npm run i18n:check:strict
  • cargo test native_file_dialogs (23 passed)
  • Imported a 600 MiB synthetic Open WebUI export in Chromium. It created 749 threads from 752 records in 51 seconds, with an 87 MiB JS heap and no page or backend errors.
  • Confirmed branches, modern and legacy tool calls, reasoning, inline images, archived chats, malformed-record recovery, and unchanged JSONL, ShareGPT, and CSV results.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@oobabooga

Copy link
Copy Markdown
Member Author

@codex review

1 similar comment
@oobabooga

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a41d9350a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +453 to +455
const renders = content.length > 0 || (role === "user" && attachments.length > 0);
const parentId = node.parentId ? (keptIdByOriginal.get(node.parentId) ?? null) : null;
if (!renders) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep the selected branch when its head is dropped

When history.currentId selects a blank or failed leaf and the same parent has another renderable response, this condition drops the selected leaf after the DFS deliberately placed it last. The final persisted message is then the other sibling, so Studio reopens—and continues from—a branch the user did not have selected in Open WebUI. Preserve a selectable placeholder/head for this case or otherwise carry the active-branch selection independently of renderable content.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced, but not changing this. The reopened head can only be the deepest surviving message on the selected path, and that message is the parent of the sibling answer, so persisting it last would put a parent after its own descendant; the repository hydration requires parents first. The other option is persisting the failed turn as an empty bubble, which is exactly what the import drops on purpose. Nothing is lost either way: the failed leaf carried no content, so the imported chat ends on the last real answer and continues from there.

@oobabooga

Copy link
Copy Markdown
Member Author

@codex review

1 similar comment
@oobabooga

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d4923b84b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +387 to +390
const parent = node.parentId ? byId.get(node.parentId) : undefined;
if (!parent || parent.id === node.id) {
node.parentId = null;
roots.push(node);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid emitting multiple roots that reload as one chain

When a damaged history contains both a normal root and an orphaned message, converting the orphan into a second null-parent root does not survive reload: runtime-provider.tsx lines 1377-1386 detects the other non-null parents and replaces every later null parent with previousId. An active orphan imported last is consequently attached beneath an unrelated branch, so continuing it sends unrelated messages as context; represent or reconcile multiple roots in a way the loader preserves.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this. The loader replaces a null parentId with the previous message for any thread that has parent ids at all, so one root per thread is a studio-wide contract, not something this import can encode differently; every alternative here either drops the orphan or splits one exported chat across threads. The recovery still keeps every message, which is the point of the orphan handling.

Comment on lines +257 to +260
if (part.type === "input_text" && typeof part.text === "string") {
resultText += part.text;
} else if (part.type === "input_image") {
const url = str(part.image_url);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve file parts returned by function calls

When a Responses-style function_call_output.output array contains an input_file part, this loop handles only input_text and input_image, so the imported tool result silently loses the file name, URL, and inline data. The repository's Responses input model accepts arbitrary content arrays and its passthrough tests include this exact input_file shape; retain a portable file part or attachment rather than dropping it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this. A tool result is stored as text plus image parts, so there is no portable place for a file part; the exported /api/v1/files/... url is dead outside Open WebUI and inline file data would be resent to the model on the next turn, which is the same reason document text is deliberately not carried over. Writing a placeholder name into the result would invent content the export did not have.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@danielhanchen

Copy link
Copy Markdown
Member

Reviewed this at head 00ea23fdf against merge base 90a6a236b. Nice piece of work, and the test coverage on the two new parsers is genuinely good. Below is before/after evidence, then what I found.

Before / after

Two independent Studio installs, one from the merge base and one from the head, same 5,297 byte pretty-printed Open WebUI export, same click through Settings > Data > Import chats. Three chats in the fixture: one with three regenerated siblings and history.currentId on the middle one, one using modern output items (reasoning, function_call plus function_call_output, an inline generated image), one using legacy <details> markup.

The result toast

toast

Recents

recents

The imported branch conversation

thread

Read from the same servers that were photographed:

merge base head
accept on the import input .jsonl,.ndjson,.csv .json,.jsonl,.ndjson,.csv
toast No conversations found in file. Imported 3 conversations to Recents.
/api/chat/count 0 3
Recents rows 0 3
messages in the branch thread 0 4

Two notes on how to read that. The file was fed to the hidden input directly, so the accept change is reported as a number rather than being what the picture shows; what the picture shows is what the same bytes do to each build. And the branch chat does reopen on the sibling currentId pointed at, which is the main thing I wanted to confirm, but the sibling counter reads 3/3 rather than 2/3, because the active branch is walked last so the kept sibling is moved to the end of its own sibling list. Content preserved, position in the list not.

Suites on the branch: npm test 2055 passed, npm run typecheck clean, npm run i18n:check:strict clean in all 12 locales, npm run build fine, cargo test native_file_dialogs 24 passed.

Worth fixing

1. An OpenAI JSONL line with per-message id and timestamp is misrouted, and a multimodal user turn is then dropped. The ("timestamp" in value && typeof value.id === "string") clause in looksLikeOpenWebUIMessage catches records the OpenAI path used to handle. Once routed here, messageParts only reads typeof message.content === "string", so an array content becomes nothing:

{ messages: [
  { id: "1", role: "user", timestamp: 1700000000,
    content: [ { type: "text", text: "hi" },
               { type: "image_url", image_url: { url: "data:image/png;base64,AAA" } } ] },
  { id: "2", role: "assistant", content: "yo", timestamp: 1700000001 } ] }

isOpenWebUIRecord returns true, and the import yields a single assistant message. The whole user turn is gone. oaiMessagesToRecords in chat-import.ts handles exactly that shape, so this is a regression for that record. Tightening the detection helps, but the cheap fix that closes the data loss regardless of routing is to let messageParts accept an array content with the same text / image_url handling.

2. unescapeHtml covers five entities, and misses the hex apostrophe. Open WebUI decodes those attributes with the full html-entities decode(), so anything outside &lt; &gt; &quot; &#39; &amp; survives into the rendered tool card. &#x27; is the common one:

arguments="{&quot;q&quot;: &quot;it&#x27;s ok&quot;}"
  -> args: { q: "it&#x27;s ok" }, result: { q: "it&#x27;s ok" }

Adding &#x27; and &#x2F; handles the realistic cases; a general numeric-entity pass would be safer.

3. Timestamp monotonicity is not guaranteed. ts = Math.max(previousTs + 1, ...) stops advancing once previousTs passes 2^53, so an out-of-range timestamp collapses a whole chat onto one createdAt:

timestamp on the first message strictly increasing distinct stamps
1.7e15 yes 4 of 4
1e16 no 1 of 4
1.7e18 (nanoseconds mistaken for ms) no 1 of 4

That matters because the comment at the top of that block is right that the depth-first order has to survive a reload, and equal stamps destroy it. Anything past 8.64e15 also renders as Invalid Date. Bounding epochMs at the top as well as the bottom (return null above 8.64e15) fixes both.

4. One record is still buffered whole. The buffer trims back to start, so the per-record ceiling is unchanged even though the whole-file one is gone. Measured on node 24 with 8 MiB chunks:

single record wall time RSS
64 MiB 0.50 s 485 MiB
128 MiB 1.26 s 855 MiB
256 MiB 3.66 s 2334 MiB
512 MiB 11.2 s RangeError: Invalid string length

The 600 MiB validation run has about 800 KiB per record, so it exercises the streaming path and does prove the whole-file limit is gone, but not this. Two exposures: a single chat over 512 MiB fails with a bare RangeError, and a top-level object wrapper such as {"version":1,"chats":[...]} is treated as one record, so it buffers the entire file and then reports "No conversations found in file." A named error above some MAX_RECORD_BYTES would turn both into something a user can act on.

5. The friendly truncation message is unreachable for a mid-record cut. yield JSON.parse(tail) runs before the sawArrayStart && !sawArrayEnd check, so an array truncated inside a record surfaces the raw V8 text:

Unterminated string in JSON at position 42 (line 1 column 43) 3 conversations were imported before it stopped.

The important half works: the three already-written chats survive, notifyChatHistoryUpdated() fires once, and the count reaches the user. A truncation between records does hit the intended message, so only the mid-record case needs wrapping.

6. Three new user-facing strings in projects-page.tsx are hardcoded English. The existing hardcoded strings there predate this PR, but toast.loading("Importing chats..."), the Importing chats: N so far (P%)... progress string and the ; N could not be saved. suffix are new here, and the matching keys were added to all 12 locales for data-tab.tsx in this same PR (settings.chat.importingChats, settings.chat.importedChatCountPartial), so they can be reused as is. Related: importedChatCountPartial has no singular form, so one conversation renders as "Imported 1 conversations".

7. The progress toast sits at 0% for any import under 25 conversations. if ((progress.imported + progress.failed) % 25 === 0) report() means a 3 record import fires exactly one progress event, the final one. bytesRead is tracked per chunk but only surfaced through report(), so the percentage stays pinned during the read. That is the case the toast exists for: a multi-GiB export made of a few very large chats shows "Importing chats: 0 so far (0%)" the whole way. Firing when bytesRead advances past the last reported value, or on a time interval, would cover it. The CSV branch never calls onBytes at all.

Smaller things

  • read_range clamps length for Vec::with_capacity but passes it unclamped to .take(length as u64). The command clamps first, so nothing is exploitable today, but the function's own invariant does not hold and the tests call it directly. Hoisting the clamp to the first line of read_range makes it self-contained.
  • A JSONL record with no created_at now gets a bare Date.now() where it used to get Date.now() + lineIdx. Combined with the updated_at bump the effect is narrower than it sounds, but records with equal message counts become exact ties on both sort keys, and neither ORDER BY has an id tiebreaker, so a defined order becomes an unspecified one. Passing the record index through as the fallback base would restore it.
  • csvToRecords gained .toLowerCase() on the role, so a CSV row with Assistant keeps its role instead of being silently rewritten to user. Looks intentional and correct, but it changes existing imports, so it is worth a line in the description.
  • An unbalanced ``` inside one <details> body opens a phantom fence that makes the next legitimate details block test true under `quotedInFence`, so a real tool call renders as raw markup. Truncated reasoning that cuts mid-fence is a realistic trigger. Nested `
    Details` are also mis-split, since the lazy `[\s\S]*?
    ` stops at the first inner close tag and Open WebUI's own tokenizer depth-counts these.
  • onMalformed increments failed, which renders as "N could not be saved". A record that failed to parse is reported as one that failed to save.
  • parseJsonLoose returns the raw string when arguments does not parse, so args can be a string where ToolCallMessagePart.args expects an object. An image_generation_call whose result is a URL rather than base64 becomes data:image/png;base64,https://..., a permanently broken image. A tool output that returned only images sets result: "", which renders as a tool card with an empty result body.
  • There is no test file for chat-import.ts. The two parsers are covered thoroughly, but the orchestration is not: the write concurrency cap, the failed accounting, the rollback in writeConversation, notifyChatHistoryUpdated after a failed read, and nativeImportSource's short-read detection are only exercised through the UI. Items 4, 5 and 7 above would each have been caught by one such test.

Things I specifically tried to break and could not: the token scoping is sound, and storing the open File rather than re-resolving the path is what makes it so, so a file swapped under the path cannot splice into the stream. 256 bits of CSPRNG entropy on the token, no path accepted from the webview, no lock nesting, offset past EOF returns empty rather than panicking. The inFlight / Promise.race pattern is correct including the const task self-reference in its own finally (peak concurrency measured at exactly 6). __proto__ in the details attributes is a no-op rather than prototype pollution. The g-flag regexes never corrupt each other's lastIndex. Cycles and the 200,000 cap both hold, including 250,000 nodes in one cycle. No catastrophic backtracking; the worst pathological message I could build cost a few hundred ms.

On the three comments you declined

The dropped selected head. You are right, and the reason is stronger than the one given. import() sets the head to messages.at(-1) since load() returns no headId, and resetHead deletes every descendant of the head. In the reported case the deepest surviving message on the selected path is the user turn, which still has a surviving child on the other branch, so making it the head would delete that sibling. No ordering keeps both. Worth noting the case is narrower than the comment implies: when the dropped leaf's parent has no other surviving child, the import already lands on the selected branch.

Multiple roots. The mechanism the comment describes is real. Two roots do get welded into one chain, because the loader backfills a null parentId with the previous message, and the imported thread then reads as one continuous conversation across a break that existed in the source. Your constraint is also real, since one record maps to one conversation. But it is not a Studio-wide contract: MessageRepository.root.children is an array and switchToBranch handles multiple roots natively. What collapses them is one line in runtime-provider.tsx that cannot tell an absent parentId from an explicit null, where chat-history-storage.ts already uses hasOwn for that same distinction. Declining the change in this file seems right to me; the fix just lives somewhere else.

File parts from function calls. You are right structurally. ThreadAssistantMessage has no attachments, fromThreadMessageLike throws on that shape, and FileMessagePart requires data, so a name-only file part is not expressible and Studio registers no file renderer for assistant content anyway. The one thing not addressed is that the file's name could be appended to the result text before it is assigned, which is the same "keep the name, drop the bytes" trade already made for documents on user turns and does not resend anything to the model. Currently the file disappears without a trace.

…mport

A Chat Completions record carrying a per-message id and timestamp satisfies
isOpenWebUIRecord, and messageParts read only string content, so a turn whose
content was the OpenAI array form was dropped entirely. Detection cannot be
made perfect, so the array form is handled here as well, the same way
oaiMessagesToRecords handles it.

epochMs now rejects a stamp past the range Date accepts. Beyond 2^53
previousTs + 1 stops advancing, which collapsed every later message in the
chat onto one createdAt and destroyed the depth-first order the surrounding
comment exists to preserve.

unescapeHtml decodes numeric character references. Open WebUI decodes those
attributes with a full html-entities pass, so an apostrophe arrives as &#x27;
as readily as &#39; and the five-entity replacement left it in the rendered
tool card.

Also: tool args are always an object, since a malformed arguments attribute
parsed loosely to a bare string where the renderer indexes an object; an
image_generation_call whose result is already a url is no longer wrapped into
a broken data url; and a tool that returned only images no longer carries an
empty result, which drew a Result heading over an empty block.
An array cut inside a record reached JSON.parse on the tail before the
closing-bracket check, so the user saw "Unterminated string in JSON at
position 42" rather than the message written for exactly this case. The
records read before the cut are still yielded, so the count of what was saved
is unchanged.

Records leave the buffer as they are emitted, so the one way to reach the
engine's maximum string length is a single record that long. Untranslated
that surfaces as a bare "Invalid string length", which says nothing about
what to do; it now names the oversized chat and suggests splitting the export.
Progress was reported only every 25 conversations, so an export made of a few
very large chats sat at "Importing chats: 0 so far (0%)" for the whole read,
which is the case the toast exists for. bytesRead was already tracked per
chunk and only needed to be surfaced.
The chunk bound was applied to Vec::with_capacity but not to take, so the
allocation was capped while the read itself was not. The command clamps before
calling, so nothing was reachable from the webview, but the function did not
hold its own invariant and the tests call it directly.
danielhanchen added a commit to shimmyshimmer/unsloth-staging-4 that referenced this pull request Aug 13, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 13, 2026
@unslothai unslothai deleted a comment from danielhanchen Aug 13, 2026
@unslothai unslothai deleted a comment from danielhanchen Aug 13, 2026
@oobabooga

Copy link
Copy Markdown
Member Author

@codex review

1 similar comment
@oobabooga

Copy link
Copy Markdown
Member Author

@codex review

Treating any unclosed ``` as a fence running to end of message made a single
stray backtick run quote everything after it, so every tool call later in the
same assistant turn was rendered as literal markup instead of a tool part. A
model writing ``` mid-sentence, or a stream cut inside one, is common enough
that this traded a rare loss for a frequent one.

Markdown opens a block fence only at the start of a line, so the unclosed form
is anchored there. The interrupted-code-block case it was added for still
holds, because that fence does begin a line.
@danielhanchen

Copy link
Copy Markdown
Member

Pushed 8c894020a on top of your 85963f0de, narrowing one part of it. Everything else in that commit matches what I had independently, including the reachability pre-pass, so I dropped my version of it.

The part I changed is the unclosed-fence rule. FENCED_CODE ending in |```[\s\S]*$ treats any unmatched backtick run as a fence to end of message, and that is reachable from ordinary output: a model writes ``` mid-sentence, or a stream is cut inside one, and everything after it is then quoted. Measured on the converter at 85963f0de:

assistant turn parts
let me compute ``` then a real tool_calls block ["text"]
reasoning whose body quotes one ``` , then a real tool_calls block ["reasoning","text"]

Both lost the tool call entirely; before the change they were ["text","tool-call","text"] and ["reasoning","tool-call","text"]. So the rule fixed literal markup inside an interrupted code block, which needs the response to be cut mid-fence and to contain Open WebUI's own markup, at the cost of dropping real tool calls in a much more common shape.

The fix keeps the intent and anchors the unclosed form to the start of a line, which is where markdown requires a block fence to begin:

/```[\s\S]*?```|~~~[\s\S]*?~~~|(?:^|\n)[ \t]{0,3}(?:```|~~~)[\s\S]*$/g

Your interrupted-code-block case still parses as code, because that fence does start a line, and the stray inline run no longer swallows anything. Both directions are pinned by tests, and your details markup inside a fence that was never closed stays code still passes.

Full suite 2070 passed, typecheck and i18n strict clean. Separately I ran a differential corpus of 37 inputs across 9 chunk sizes against the merge base, a 1500-case tokenizer fuzz, and the three browser engines (Chromium, Firefox, WebKit) through real File objects; all still clean at this head.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e42d60c7eb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +323 to +324
let file =
File::open(&path).map_err(|error| format!("Failed to open {}: {error}", path.display()))?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Read the import size from the opened file handle

If the selected path is replaced between fs::metadata(&path) and File::open(&path), the token references the replacement while size still describes the previous file. A larger replacement is then silently truncated to the stale length, while a smaller one fails partway through import; derive metadata from the opened File so the advertised size and streamed bytes always refer to the same object.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9476d8e: the size comes from the opened handle, so the number that bounds the read and the bytes that are read describe the same object. The is_file check on the path stays ahead of the open, because that is what keeps the directory error the same on Windows, and it is repeated on the handle.

Comment on lines +190 to +193
// The array's own closing bracket: the file is complete from here on.
if (code === CLOSE_BRACKET && sawArrayStart) sawArrayEnd = true;
// Commas, newlines, and any stray scalar between records: nothing to import.
scan++;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject records after the top-level array closes

When a .json export contains non-whitespace data after its closing bracket, the scanner sets sawArrayEnd but continues looking for { or [. A trailing object such as [{...}]\n{...} is therefore imported as another conversation even though it is outside the export's top-level array, making a corrupted or concatenated file appear valid and silently importing unintended data. Stop after the closing bracket or reject any subsequent non-whitespace content.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9476d8e: past the closing bracket only whitespace is accepted and anything else throws, so a concatenated file is refused rather than half-imported. Same rule as the missing bracket at end of input.

Comment on lines +273 to +276
const source = Array.isArray(item.summary)
? item.summary
: Array.isArray(item.content)
? item.content

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fall back from an empty reasoning summary to content

When a Responses-style reasoning item contains summary: [] alongside populated content, the presence of the empty summary array selects it and prevents the content fallback from running. The imported conversation consequently loses all reasoning from that item; choose the first source that actually contains usable text, or merge the two sources.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9476d8e: the source is whichever of summary and content actually holds text, with summary still winning when both do. That is the shape when summaries are off, which is the common one.

danielhanchen added a commit to shimmyshimmer/unsloth-staging-4 that referenced this pull request Aug 13, 2026
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

1 similar comment
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@oobabooga

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 9476d8ebb4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danielhanchen
danielhanchen merged commit 42c8671 into unslothai:main Aug 13, 2026
4 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants