fix(notebook): handle plain-string outputs in _extract_output - #604
Closed
wallidsaydi-creator wants to merge 1 commit into
Closed
fix(notebook): handle plain-string outputs in _extract_output#604wallidsaydi-creator wants to merge 1 commit into
wallidsaydi-creator wants to merge 1 commit into
Conversation
nbformat v4 allows stream.text and output.data['text/plain'] as either a list of strings or a plain string. _extract_output returned the raw value, and the caller's list += iterated characters, exploding every string-form output into one-character-per-line garbage in the LLM-facing digest. Coerce string form via splitlines(), which also renders multi-line string outputs line-for-line like the list form. Fixes #603
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Converts the fix offered in #603 into a reviewable PR.
process_notebookcorrupts cell outputs into one-character-per-line garbage whenever an output stores its text as a plain string rather than a list of strings — which the nbformat v4 schema explicitly allows for bothstream.textandoutput.data["text/plain"](the schema permitsstring | array of stringsfor multiline vs single-line storage).A normal output like
hello worldcurrently renders as:burning tokens and destroying readability for any notebook produced by a tool that serializes single-line outputs as strings (common in programmatic notebook generation and some exporter paths).
Root cause
_extract_outputinsrc/gitingest/utils/notebook.pyreturned the raw value. The caller (_process_cell) then doesraw_lines += _extract_output(output)— when the value is astr,list.__iadd__iterates characters, and each character becomes its own#-prefixed line.cell["source"]in the same file already handles both forms correctly, so_extract_outputwas the only unguarded site.Fix
Coerce the string form via
splitlines()in both branches. This also restores correct rendering for multi-line string-form outputs, matching the list-form behavior line-for-line.Verification
main(4e259a0): string-form stream + execute_result outputs explode into per-character lines (regression test fails with the exact signature below).# hello world/# 42on single lines; multi-line string form splits correctly.test_process_notebook_string_form_outputintests/test_notebook_utils.py— proven RED on stock (stash control) and GREEN with the patch.152 passed(151 stock + this test). The only failures anywhere are 3 pre-existing network-dependent bitbucket cases intest_git_host_agnostic.pythat fail identically on unpatched stock (verified via stash control).ruff checkparity with stock (no new findings; the 2 CPY001 copyright notices are pre-existing on both trees).Fixes #603
Context: I run FreshContext — a $5 pack that keeps AI coding agents off stale docs and deprecated patterns. Recently credited in llm-docs-builder v1.0.0 for a similar LLM-pipeline correctness fix, and have a merged fix in cloudflare-docs#32985.