Conversation
get_tree returns one node shape in local and cloud mode: {title, node_id,
start_index, end_index, summary, text, nodes}. page_index and
prefix_summary no longer appear. The SDK only renames fields on the way
out, so a document indexed before keeps its own ranges and summaries.
New local indexes, standard and flash:
- A parent whose first child starts on a later page gets a first child
"<parent title> (intro)" that holds those pages.
- A parent's range covers its whole subtree, and its summary is written
from its children's summaries. Standard mode now summarizes with
summarize_tree, as flash does.
- A node the model leaves unsummarized falls back to its subsection titles
or its own text.
- The standard large-node split acts on leaves only.
A node's text is its own pages. A parent's runs onto the page its first
child starts on, and is empty when its intro holds those pages.
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. |
Flash expand priced a candidate level without the intro attach_children then adds. An intro sharing its first child's page could make the committed node cost exactly its span, so the next merge folded it back after its summary was final, and the whole index raised "dropped or changed after it was marked final" (PRML 5.1, the 2023 annual report). expand_cost now prices the level with its intro. heading_at_page_start took any first line containing the heading as the heading opening its page. A running header repeating the section title cut intros and the Preface a page early, so the opening text above the real heading was in no node. The first line must now start with the heading, no later line may start with it, and a heading with no Latin letter is never placed. An uncertain page is shared. Expand on an intro or the Preface could list the heading printed on its shared last page (the next node's) or atop it (its parent's) as a new child. A proposal whose page and title, leading number aside, match a node already in the tree is dropped. The local get_document_structure tool read the stored tree through LocalAPI.raw_tree and missed get_tree's end < start clamp. raw_tree is gone; both modes read client.get_tree. A test runs the standard tree_parser path end to end: intro insertion before and after the large-node split, and the covering pass on the default tail.
50caf15 dropped a proposed child whose page and title, leading number aside, matched any node in the live tree. Sibling expansions run concurrently, so the result hung on reply order: when Methods' reply landed before Results', Methods kept "3.1 Discussion" from their shared page and Results lost its own. Stripping every leading number also took "3.2 Results" for its parent "3 Results" and dropped it. A proposal is now dropped when it already is a node, in the tree as expand found it or made by the node's own ancestors, which no concurrent branch can change; or when, on a page the node shares, it is printed above the node's heading or at and below the next node's. A heading not found on its page decides nothing. A leading number is ignored only when one of the two titles lacks it. The check runs before the empty-reply retry, and the tree is read once per pass instead of once per node. A summary reply counted as answered whenever it was non-empty, so a model returning {"summary": ""} everywhere stored a document of fallback summaries without the all-failed error. It now counts only when a summary parses out of it. post_processing ended a TOC item a page before its start when the next item opened the same page; the end is now at least the start.
heading_at_page_start never places a heading without a Latin letter, so a bare "2" (often a page number) shares its page; nothing tested that. The add_intro_nodes idempotency check compared the tree with itself, the same object the call returns, so it could not fail; it now compares a second pass over a copy with the first.
The ownership filter ran inside the try that absorbs failed model calls, so a bug in it would be logged as a model error, retried, and leave the node silently unexpanded. It now runs after the call, where an error surfaces. The neighbor test now also pins the retry: a reply the filter empties is asked again. own_children drops a guard for lines expand always passes.
A mutation pass over the filter found 13 of 21 mutants surviving the suite. Each part now has a test that fails without it: the snapshot of known nodes (Results' run-in heading leaves only the tree to recognize its subsection), the lineage of nodes this pass made, the ancestors and next node each new child is handed, filtering of cached headings, the occurrence each anchor reads (last line of a repeated next heading, last line of a child mentioned above), `>=` for a variant printed on the next heading's own line, key_items for a same-page fusion, and the clamp for a TOC item listed out of order.
This branch has not been deployed
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
1 participant
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.
Update (Oct 5): four more commits from a second review of this view: ff4cfd0, cdcaf65, c29ffa7 and 0be2352. Open them from the Commits tab.
post_processingno longer ends a TOC item before its start page when the next item opens the same page.Update (Oct 4): this branch now has a second commit, 50caf15, with fixes from reviewing this view. Open it from the Commits tab to review it on its own. The rest of this description covers 6d23caf as shipped. 50caf15 changes:
heading_at_page_startno longer takes a running header that repeats the heading as the heading opening its page. A heading now needs a Latin letter to be placed; digits alone no longer count.get_document_structuretool readsclient.get_tree, with its end < start clamp.LocalAPI.raw_treeis removed.tree_parserpath end to end.Frozen-base review view of #541 as squash-merged to main (6d23caf), released in v0.2.21. Base
review/base-f0d67c1is main right before that merge, so this diff is exactly what #541 shipped. Against #541's own PR diff, the added and removed lines are identical; one context line differs, from #542.Never merge. Fixes found here go to main through their own PRs.
What shipped
Read side.
get_treereturns one node shape in local and cloud mode:{title, node_id, start_index, end_index, summary, text, nodes}.page_indexandprefix_summaryno longer appear, which is breaking for code that reads them.utils.unify_treeonly renames fields (page_index→start_index,prefix_summary→summary). It never adds or moves a node.end_index, each node's end is filled from the next node's start. This costs one extra metadata request.Index side, new local indexes in standard and flash:
"<parent title> (intro)"holding those pages, or"Intro"when the parent has no title. This is decided by page.summarize_tree, as flash does: per node, deepest first, under the concurrency cap. A parent waits only for its own children.fallback_summary. A run in which no call is answered still raises.Page boundaries share, never drop. When a cut can't tell where a heading sits on a page, the page goes to both sides.
own_pages). It is empty when the parent's intro holds those pages (is_intro).add_node_textandadd_node_text_with_labelscut by the same rule.heading_at_page_start).Known issues, already tracked (no need to re-report)
tree_optimize.normalizekeeps only[a-z0-9], so a CJK, Arabic or Hindi title normalizes to"".propose_childrenandchildren_from_cache, such a node title equals every non-Latin proposal, so the node gets zero children.seen.get_treetext comes from PyPDF2 (pages.json), while flash builds the tree and writes summaries from pdfium text. The two can extract a page differently._heading_appears_at_page_top(flash/outline_assembly/assembly.py) returns True ("drop the page") when a heading has no group slot or page. This contradicts share-never-drop in theory. A probe hit it 0 times in 514 headings over 12 PDFs.flash/outline/tree.pybuild_treeis exported but never called. It ends each section atnext.start - 1.utils.get_intro_textandSUMMARY_INTRO_MAX_PAGESalways yield""on a tree built with intro nodes, since the intro holds every opening. Cleanup candidate.Review focus
get_treebreaking change, andunify_treeon old cloud trees (noend_index,prefix_summary) vs new ones.get_node,get_node_parent,get_node_path,get_node_map,create_node_mapping) on the new shape. This covers intro nodes and the"<id>.0"ids flash expand'sattach_childrengives a new parent's intro.summarize_treescheduling and the fallback, and the leaf-only split.own_pages,is_intro,add_node_text*).Tests (from #541)
tests/test_tree_format.py: 10 tests, each failing on main before the merge.test_local_chat, whose failures come from an httpx environment issue and also fail on main. 601 passed and 218 skipped without frameworks.get_treereturns it unchanged.