diff --git a/internal/config/config_treewalk_test.go b/internal/config/config_treewalk_test.go index 10143b8..0a57ae1 100644 --- a/internal/config/config_treewalk_test.go +++ b/internal/config/config_treewalk_test.go @@ -18,13 +18,13 @@ func TestForwardTreeWalkMaxCitations(t *testing.T) { os.Setenv("VLE_RETRIEVAL_TREEWALK_MAX_CITATIONS", prevVLE) }() - // Engine default is 3 with no env set. + // Engine default is 6 with no env set. os.Unsetenv("VLS_RETRIEVAL_TREEWALK_MAX_CITATIONS") os.Unsetenv("VLE_RETRIEVAL_TREEWALK_MAX_CITATIONS") cfg := Default() applyEnvOverrides(&cfg) - if cfg.Engine.Retrieval.TreeWalk.MaxCitations != 3 { - t.Errorf("default max_citations = %d, want 3", cfg.Engine.Retrieval.TreeWalk.MaxCitations) + if cfg.Engine.Retrieval.TreeWalk.MaxCitations != 6 { + t.Errorf("default max_citations = %d, want 6", cfg.Engine.Retrieval.TreeWalk.MaxCitations) } // VLE_ alone forwards through. @@ -43,12 +43,12 @@ func TestForwardTreeWalkMaxCitations(t *testing.T) { t.Errorf("VLS_ should win: max_citations = %d, want 1", cfg3.Engine.Retrieval.TreeWalk.MaxCitations) } - // Garbled value preserves the engine default (3), does not zero it. + // Garbled value preserves the engine default (6), does not zero it. os.Unsetenv("VLS_RETRIEVAL_TREEWALK_MAX_CITATIONS") os.Setenv("VLE_RETRIEVAL_TREEWALK_MAX_CITATIONS", "heaps") cfg4 := Default() applyEnvOverrides(&cfg4) - if cfg4.Engine.Retrieval.TreeWalk.MaxCitations != 3 { - t.Errorf("garbage value should preserve default 3, got %d", cfg4.Engine.Retrieval.TreeWalk.MaxCitations) + if cfg4.Engine.Retrieval.TreeWalk.MaxCitations != 6 { + t.Errorf("garbage value should preserve default 6, got %d", cfg4.Engine.Retrieval.TreeWalk.MaxCitations) } } diff --git a/pkg/config/config.go b/pkg/config/config.go index 12dfe0c..beaefee 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -760,7 +760,7 @@ func Default() Config { Enabled: true, MaxHops: 8, PageContentLimit: 16000, - MaxCitations: 3, + MaxCitations: 6, }, }, Ingest: IngestConfig{ diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 5c64db3..9a3bb34 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -684,8 +684,8 @@ func TestTreeWalkDefaults(t *testing.T) { if cfg.Retrieval.TreeWalk.PageContentLimit != 16000 { t.Errorf("page_content_limit = %d, want 16000", cfg.Retrieval.TreeWalk.PageContentLimit) } - if cfg.Retrieval.TreeWalk.MaxCitations != 3 { - t.Errorf("max_citations = %d, want 3", cfg.Retrieval.TreeWalk.MaxCitations) + if cfg.Retrieval.TreeWalk.MaxCitations != 6 { + t.Errorf("max_citations = %d, want 6", cfg.Retrieval.TreeWalk.MaxCitations) } if cfg.Retrieval.TreeWalk.Model != "" { t.Errorf("model default should be empty (inherit), got %q", cfg.Retrieval.TreeWalk.Model) @@ -802,8 +802,8 @@ func TestTreeWalkEnvOverrideRejectsBad(t *testing.T) { if cfg.Retrieval.TreeWalk.PageContentLimit != 16000 { t.Errorf("garbage page_content_limit env should preserve default, got %d", cfg.Retrieval.TreeWalk.PageContentLimit) } - if cfg.Retrieval.TreeWalk.MaxCitations != 3 { - t.Errorf("garbage max_citations env should preserve default 3, got %d", cfg.Retrieval.TreeWalk.MaxCitations) + if cfg.Retrieval.TreeWalk.MaxCitations != 6 { + t.Errorf("garbage max_citations env should preserve default 6, got %d", cfg.Retrieval.TreeWalk.MaxCitations) } } diff --git a/pkg/parser/empty_sections_test.go b/pkg/parser/empty_sections_test.go new file mode 100644 index 0000000..145dce4 --- /dev/null +++ b/pkg/parser/empty_sections_test.go @@ -0,0 +1,88 @@ +package parser + +import ( + "strings" + "testing" +) + +// TestFoldEmptyLeafSections proves the post-parse pass eliminates empty +// "structural" leaf fragments — the multi-line display-title remnants and +// running headers that otherwise ship as sections returning empty content +// to the browser / API — while preserving their title text on the sibling +// body they head, and never touching internal nodes (empty headings that +// carry real sub-sections). +func TestFoldEmptyLeafSections(t *testing.T) { + t.Parallel() + + t.Run("empty leaf folds its title into the following sibling", func(t *testing.T) { + in := []Section{ + {Level: 1, Title: "How Vectorless", Content: "", PageStart: 3, PageEnd: 3}, + {Level: 1, Title: "Body", Content: "Vectorless walks the document tree.", PageStart: 3, PageEnd: 4}, + } + out := foldEmptyLeafSections(in) + if len(out) != 1 { + t.Fatalf("want 1 section (empty leaf folded away), got %d: %+v", len(out), out) + } + if strings.Contains(out[0].Content, "walks the document tree") == false { + t.Errorf("body content lost: %q", out[0].Content) + } + if !strings.Contains(out[0].Content, "How Vectorless") { + t.Errorf("folded title not preserved in content: %q", out[0].Content) + } + if out[0].PageStart != 3 { + t.Errorf("PageStart should union to 3, got %d", out[0].PageStart) + } + }) + + t.Run("consecutive empty fragments accumulate onto one body", func(t *testing.T) { + secs := []Section{ + {Level: 1, Title: "THE SYSTEM, END-TO-END", Content: "", PageStart: 2, PageEnd: 2}, + {Level: 1, Title: "How Vectorless", Content: "", PageStart: 2, PageEnd: 2}, + {Level: 1, Title: "actually works.", Content: "", PageStart: 2, PageEnd: 2}, + {Level: 1, Title: "Intro", Content: "The system reads structure first.", PageStart: 2, PageEnd: 3}, + } + out := foldEmptyLeafSections(secs) + if len(out) != 1 { + t.Fatalf("want 1 section, got %d: %+v", len(out), out) + } + for _, frag := range []string{"THE SYSTEM", "How Vectorless", "actually works", "reads structure first"} { + if !strings.Contains(out[0].Content, frag) { + t.Errorf("fragment %q missing from folded content: %q", frag, out[0].Content) + } + } + }) + + t.Run("empty internal node (heading with children) is preserved", func(t *testing.T) { + in := []Section{ + {Level: 1, Title: "Chapter", Content: "", Children: []Section{ + {Level: 2, Title: "Sub", Content: "real body", PageStart: 5, PageEnd: 5}, + }}, + } + out := foldEmptyLeafSections(in) + if len(out) != 1 || len(out[0].Children) != 1 { + t.Fatalf("internal node must survive with its child, got %+v", out) + } + }) + + t.Run("trailing empty fragment with no following body attaches to previous", func(t *testing.T) { + in := []Section{ + {Level: 1, Title: "Body", Content: "content here", PageStart: 1, PageEnd: 1}, + {Level: 1, Title: "Dangling Header", Content: "", PageStart: 2, PageEnd: 2}, + } + out := foldEmptyLeafSections(in) + if len(out) != 1 { + t.Fatalf("want 1 section, got %d", len(out)) + } + if !strings.Contains(out[0].Content, "Dangling Header") { + t.Errorf("trailing title lost: %q", out[0].Content) + } + }) + + t.Run("lone empty leaf with no siblings is kept (nothing to fold into)", func(t *testing.T) { + in := []Section{{Level: 1, Title: "Only", Content: ""}} + out := foldEmptyLeafSections(in) + if len(out) != 1 { + t.Fatalf("lone empty leaf should be kept, got %d", len(out)) + } + }) +} diff --git a/pkg/parser/pdf.go b/pkg/parser/pdf.go index d930162..168397a 100644 --- a/pkg/parser/pdf.go +++ b/pkg/parser/pdf.go @@ -424,10 +424,32 @@ func (p *PDF) parseDoc(_ context.Context, buf []byte) (*ParsedDoc, error) { // depth relative to the font-derived level. We only ever DEEPEN // (never change the base level), so top-level headings — numbered // "3" or not ("Abstract") — stay siblings at the same font level. + newLevel := lvl if nd, ok := numberedHeadingDepth(text); ok && nd > 1 { - lvl += nd - 1 + newLevel += nd - 1 } - current = &flat{level: lvl, title: text} + // Multi-line display heading: a run of heading rows at the SAME + // level, on the SAME page, with NO body text accrued between them + // is one visual heading wrapped across lines — e.g. a hero title + // "THE SYSTEM, END-TO-END" / "How Vectorless" / "actually works." + // Left un-merged, each line becomes its own heading flat whose + // body is empty, shipping as a "purely structural" Section that + // returns empty content to the section browser / API. Fold the + // continuation line into the current heading instead so the body + // that follows attaches to the whole title and no empty fragment + // section is emitted. We only merge when the current flat is + // itself an as-yet-empty heading at the same level on the same + // page — never across a level change (real nesting) or once body + // text has begun. + if current.level > 0 && newLevel == current.level && + current.title != "" && + strings.TrimSpace(current.body.String()) == "" && + (current.pageEnd == 0 || row.page == 0 || row.page == current.pageEnd) { + current.title = strings.TrimSpace(current.title + " " + text) + touch(current, row.page) + continue + } + current = &flat{level: newLevel, title: text} touch(current, row.page) flats = append(flats, current) continue @@ -509,6 +531,11 @@ func (p *PDF) parseDoc(_ context.Context, buf []byte) (*ParsedDoc, error) { }} } + // Drop empty "structural" leaf fragments (multi-line title remnants, + // running headers) so no section returns empty content to the browser / + // API; their titles ride onto the sibling body they head. + rootSec.Children = foldEmptyLeafSections(rootSec.Children) + // Internal sections inherit the union of their children's page ranges // so callers reading the outline can still cite a page span. propagateSectionPages(rootSec.Children) @@ -550,6 +577,115 @@ func (p *PDF) resolvedParseTimeout() time.Duration { return p.ParseTimeout } +// foldEmptyLeafSections eliminates leaf sections (no children) whose +// content is empty. A PDF whose heading detector fires on a fragment that +// carries no body — a lone display-title line the row-merge didn't catch, +// a repeated running header, a stray bold word — otherwise ships as a +// "purely structural" Section that returns empty content to the section +// browser and the /v1/sections/{id} API, which reads as a broken/empty +// part of the document. +// +// The fold is lossless: an empty leaf's TITLE is preserved by attaching it +// as a bold markdown heading line to the content of the sibling it heads — +// the NEXT sibling when one exists (the body it introduces), else the +// PREVIOUS sibling. Consecutive empty leaves accumulate and flush together +// into the first sibling that can carry them, so a multi-line title that +// escaped the row-merge still lands as one heading block above its body. +// Page ranges union so citations stay correct. Internal nodes (sections +// WITH children) are recursed into but never dropped — an empty heading +// with real sub-sections is legitimate structure whose content lives in +// its children. +// +// A lone empty leaf with no siblings to carry its title is kept as-is +// (dropping it would erase the only trace of that heading); this is the +// rare degenerate case and harms nothing. +func foldEmptyLeafSections(sections []Section) []Section { + for i := range sections { + if len(sections[i].Children) > 0 { + sections[i].Children = foldEmptyLeafSections(sections[i].Children) + } + } + + // Collect the indices of empty leaves eligible to fold. A section is an + // empty leaf when it has no children and no non-whitespace content. + emptyLeaf := func(s *Section) bool { + return len(s.Children) == 0 && strings.TrimSpace(s.Content) == "" + } + // Nothing to do unless there is at least one empty leaf AND at least one + // sibling that can carry a folded title. + empties, carriers := 0, 0 + for i := range sections { + if emptyLeaf(§ions[i]) { + empties++ + } else { + carriers++ + } + } + if empties == 0 || carriers == 0 { + return sections + } + + out := make([]Section, 0, len(sections)) + var pendingTitles []string + var pendStart, pendEnd int + takePending := func(target *Section) { + if len(pendingTitles) == 0 { + return + } + var b strings.Builder + for _, t := range pendingTitles { + b.WriteString("**") + b.WriteString(t) + b.WriteString("**\n\n") + } + b.WriteString(target.Content) + target.Content = strings.TrimSpace(b.String()) + target.PageStart = minNonZero(pendStart, target.PageStart) + if pendEnd > target.PageEnd { + target.PageEnd = pendEnd + } + pendingTitles = nil + pendStart, pendEnd = 0, 0 + } + + for i := range sections { + s := sections[i] + if emptyLeaf(&s) { + if t := strings.TrimSpace(s.Title); t != "" { + pendingTitles = append(pendingTitles, t) + pendStart = minNonZero(pendStart, s.PageStart) + if s.PageEnd > pendEnd { + pendEnd = s.PageEnd + } + } + continue // drop the empty leaf; its title rides on pendingTitles + } + // A real section: it carries any pending titles as heading prefix + // (these are the fragments it follows). + takePending(&s) + out = append(out, s) + } + + // Any titles still pending had no following carrier — attach them to the + // last emitted sibling as a trailing heading block so nothing is lost. + if len(pendingTitles) > 0 && len(out) > 0 { + last := &out[len(out)-1] + var b strings.Builder + b.WriteString(last.Content) + for _, t := range pendingTitles { + b.WriteString("\n\n**") + b.WriteString(t) + b.WriteString("**") + } + last.Content = strings.TrimSpace(b.String()) + if pendEnd > last.PageEnd { + last.PageEnd = pendEnd + } + last.PageStart = minNonZero(pendStart, last.PageStart) + } + return out +} + // propagateSectionPages fills internal-node PageStart/PageEnd from the union // of descendant leaf ranges where the internal node didn't have its own // (because its body was empty / hoisted into children). Leaves keep their @@ -1268,7 +1404,9 @@ func parsePDFWithOutline(outline pdflib.Outline, rows []pdfRow) (*ParsedDoc, boo title = rootSec.Children[0].Title } - // Propagate page ranges so internal nodes span their children. + // Fold empty leaf fragments, then propagate page ranges so internal + // nodes span their children. + rootSec.Children = foldEmptyLeafSections(rootSec.Children) propagateSectionPages(rootSec.Children) return &ParsedDoc{ diff --git a/pkg/retrieval/treewalk_strategy.go b/pkg/retrieval/treewalk_strategy.go index d3600b9..2797309 100644 --- a/pkg/retrieval/treewalk_strategy.go +++ b/pkg/retrieval/treewalk_strategy.go @@ -140,15 +140,23 @@ const defaultTreeWalkMaxHops = 8 // excerpt. Matches TreeWalk's reference behaviour. const defaultPageContentLimit = 16000 -// defaultTreeWalkMaxCitations bounds the FINAL cited-range set. Three -// is generous for the answer-spans-one-place common case (where ONE is -// ideal) while still allowing a genuinely multi-location answer (e.g. a -// 10-K figure cross-referenced between the income statement and a -// footnote) to cite two or three distinct ranges. The FinanceBench -// signal that motivated the cap: confident single-pick = f1 1.0, -// 5-range spray = f1 0. Capping at 3 keeps the legitimate multi-range -// case while removing the long tail of low-confidence noise. -const defaultTreeWalkMaxCitations = 3 +// defaultTreeWalkMaxCitations bounds the FINAL cited-range set. The cap +// is a CEILING, not a target: it only bites when the model emits MORE +// distinct ranges than this. The prompt still drives the model to the +// minimal sufficient set (usually one), so a confident single-pick answer +// still yields ONE citation and the FinanceBench precision behaviour is +// unchanged — the signal that motivated a cap at all (confident single = +// f1 1.0, 5-range spray = f1 0) is enforced by the prompt + confidence +// gating, not by clamping legitimate multi-location answers. +// +// It is raised from 3 to 6 so richer, genuinely multi-location answers — +// a synthesis question over a whitepaper that draws on the intro, a +// method section and a results table; a comparison spanning several parts +// of a report — surface every location they actually rely on rather than +// being truncated to three. Six is still well below the "spray" regime the +// prompt/confidence machinery suppresses, so precision-critical single-pick +// answers are untouched while multi-source answers become fully traceable. +const defaultTreeWalkMaxCitations = 6 // strategyNameTreeWalk is the stable identifier for config // (retrieval.strategy: treewalk) and telemetry.