Addresses items 9-13 and a/b/c/f from the max-effort review of PR #272.
- pdf.py: image colorspace check was `pix.n > 4`, which treats CMYK-without-
alpha (n==4, same as RGBA) as not needing RGB conversion; pix.save() as .png
then raises "unsupported colorspace", silently dropped by the surrounding
except. Fixed to `pix.n - pix.alpha >= 4` (correctly converts CMYK, leaves
RGBA untouched).
- pipeline.py: detect_strategy([]) (an empty/whitespace-only source file)
returned "content_based", routing into the PDF-oriented TOC-detection
pipeline -- wasting a real LLM call before raising IndexingError. Empty
node lists now route to level_based, whose build_tree_from_levels([])
returns an empty structure instantly with zero LLM calls.
- page_index.py (shim): pageindex/__init__.py binds the canonical `page_index`
function as the package attribute, but this file is ALSO a real submodule
of the same name -- importing it anywhere (import machinery, unconditional)
overwrites that attribute with the module object, breaking
`from pageindex import page_index; page_index(x)` for the rest of the
process. Made the shim module itself callable (delegates to the real
function via a ModuleType subclass), so whichever object ends up in that
slot is callable regardless of import order.
- storage/sqlite.py: create_collection let a raw sqlite3.IntegrityError escape
on a duplicate name (new CollectionAlreadyExistsError); the collections
table's CHECK constraint only validated the name's first character (GLOB
'*' is a wildcard, not a regex quantifier over the preceding class) --
fixed to validate the whole string, and SQLiteStorage now also validates in
Python (it's a public StorageEngine usable directly, bypassing
LocalBackend's own check).
- tests/test_review_fixes_2.py: two tests used a ContentNode with no `level`
set, so build_index took the content_based path and made real (retried,
slow, and -- with a valid key -- billable) LLM calls instead of testing the
text-stripping logic they claimed to. Mocked out _content_based_pipeline.
- retrieve.py: _parse_pages/_get_pdf_page_content were independent copies of
the canonical parse_pages/get_pdf_page_content that had already drifted
(missing the p>=1 filter and 1000-page DoS cap) -- delegate to canonical
now, so the legacy pageindex.get_page_content path can't silently regress
again.
- parser/markdown.py: a leading UTF-8 BOM broke first-header detection
(not whitespace, .strip() doesn't remove it) -- decode utf-8-sig. Only
backtick fences were recognized as code blocks, so a '#'-prefixed line
inside a ~~~-fenced block (valid CommonMark) was misparsed as a heading --
recognize both fence styles.
- run_pageindex.py: --if-thinning wasn't migrated to the bare-flag +
legacy-yes/no convention the other four --if-add-* flags got; bare usage
raised an argparse error and it never went through the shared coercion.
- types.py: DocumentDetail's `structure` field was inside the class's
total=False body, so TypedDict rules made it optional even though every
backend always populates it. Split into a required base class.
Adds regression tests for all of the above. Full suite: 244 passed, 2 skipped
(one pre-existing, unrelated flaky cloud-streaming test).
Claude-Session: https://claude.ai/code/session_01Kx5DgKbhK1N8autqXH8SmS
Addresses items 4-8 from the max-effort review of PR #272 (VectifyAI/PageIndex#272).
- agent.py: wrap_with_doc_context() strips '<'/'>' from doc_name/doc_description
(untrusted: unsanitized filename / LLM-generated from document content) before
inserting them into the <docs>...</docs> block, so embedded content can never
form a literal </docs> that closes the delimiter early and escapes the
untrusted-data boundary SCOPED_SYSTEM_PROMPT relies on. Deterministic
per-field transform, doesn't touch the (cacheable) static system prompt.
- ConfigLoader.load() (legacy 0.2.x compat) now routes merged overrides through
IndexConfig before returning, so a legacy 'no' string gets pydantic's bool
coercion instead of surviving as a truthy non-empty string — page_index_main's
bare `if opt.if_add_node_summary:` checks (changed from `== 'yes'` elsewhere
in this PR) were silently inverting caller intent and firing unwanted billed
LLM calls.
- verify_toc, process_large_node_recursively, tree_parser,
generate_summaries_for_structure, generate_summaries_for_structure_md: added
return_exceptions=True to their asyncio.gather calls (llm_completion/
llm_acompletion raise RuntimeError on retry exhaustion, added earlier in this
PR), each with a degrade path matching the pattern already used by sibling
hardened gathers in the same files. One transient LLM failure no longer
aborts the whole document's indexing.
- _llm_semaphore is now a true process-wide ceiling (threading.Semaphore,
shared across every thread/event loop) instead of one asyncio.Semaphore per
event loop -- concurrently indexing N documents on N threads no longer
multiplies the effective cap by N. A max_concurrency_scope() override is
layered as a second, nested, per-loop restriction that can only tighten the
effective cap within the ceiling, never widen past it.
- set_llm_params() mutated a bare process-wide dict with no per-call isolation,
unlike max_concurrency which already had ContextVar scoping. Added
llm_params_scope() (mirrors max_concurrency_scope) + IndexConfig.llm_params,
wired into build_index() the same way max_concurrency already was, so
concurrent indexing jobs with different llm kwargs don't leak into each
other.
Adds regression tests for all five. Full suite: 221 passed, 2 skipped (one
pre-existing, unrelated flaky cloud-streaming test intermittently fails on
rerun; confirmed independent of this change).
Claude-Session: https://claude.ai/code/session_01Kx5DgKbhK1N8autqXH8SmS
build_tree_from_levels seeds every node's 'text', but the removal was gated on
`strategy != "level_based"`, so a default Markdown index (level_based,
if_add_node_text=False) leaked each node's full text into
get_document_structure / storage — inconsistent with if_add_node_text=False,
the README, and the legacy md_to_tree.
Move the strip to the end of build_index and apply it to BOTH strategies:
summary/description generation runs first and still sees the text, and
create_clean_structure_for_description doesn't depend on text. From Codex
review of PR #272.
Claude-Session: https://claude.ai/code/session_01Kx5DgKbhK1N8autqXH8SmS
The previous approach (bounded_gather building a fresh semaphore per call)
did NOT compose: the indexing call graph nests gathers
(tree_parser -> process_large_node_recursively -> recurse, plus each node's
check_title gather), and each level got its own independent cap. Peak
in-flight LLM calls grew ~N^depth, so a deep/wide document still exhausted
file descriptors (Errno 24) — the exact failure the cap was meant to
prevent — while the flat summary phase was over-serialized. Naively sharing
one semaphore across gather levels would instead deadlock (a parent holds a
slot while awaiting children that need slots).
Move the throttle to the single chokepoint every LLM call funnels through,
llm_acompletion: one shared semaphore per event loop, acquired only around
the litellm.acompletion network call. This gives a true global cap that
composes across any nesting and can't deadlock (a parent awaiting children
holds no slot). bounded_gather is gone; the call sites revert to plain
asyncio.gather.
Also:
- Reject bool in max_concurrency validation (bool is an int subclass, so
set_max_concurrency(True) / IndexConfig(max_concurrency=True) previously
became Semaphore(1) and silently serialized). Shared _validate_max_concurrency
+ a pydantic field_validator.
- Guard check_title_appearance_in_start_concurrent against an out-of-range
or 0 physical_index (LLM can emit one): it was dereferenced during task
construction, outside the gather's return_exceptions protection, aborting
the whole build; 0 silently wrapped to the last page. Now marked 'no'.
- Propagate contextvars into agent.py's worker-thread run (mirrors
pipeline._run_async) so ContextVar settings stay consistent.
- Tests rewritten to cover the nested case the old flat tests missed, the
leaf-level throttle in llm_acompletion, bool rejection, and the
out-of-range physical_index guard.
Claude-Session: https://claude.ai/code/session_01Kx5DgKbhK1N8autqXH8SmS
