Store answer: reason over structure+summaries, then full section content#58
Merged
Conversation
Replace the store answer's map stage: instead of fanning the section-selection strategy across documents (retrieve-then-generate, chunk-shaped), run the FULL agentic tree-walk on EACH document — read its structure, navigate hop by hop to a grounded per-document answer + page citations — then synthesise the per-document answers into one. Every document is now reasoned over exactly like /v1/answer/treewalk; no chunking, no embeddings anywhere in the collection path. - max_depth controls the per-document tree-walk hop budget (0 = the engine's full/default depth), as requested. - max_docs bounds how many documents are tree-walked (0 = all) — a cost valve for large collections; the per-doc walks run with bounded concurrency. - Citations are one-per-contributing-document, carrying the document's cited page span + overlapping section ids (from the tree-walk's CitedPages) and a short quote from its answer. Refusals/empty per-doc answers are dropped before synthesis. Returns 501 unless the tree-walk strategy + an LLM are configured.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe /v1/answer/store handler is refactored from a multi-document evidence gatherer/reducer to a per-document tree-walk map step followed by a synthesis reduce step. Dependencies switch from storage/MultiDoc to storeTreeLoader and TreeWalkStrategy, citations are rebuilt per-document, and router wiring and tests are updated accordingly. ChangesAnswer Store Tree-Walk Refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as HandleAnswerStore
participant Map as mapTreeWalk
participant Loader as storeTreeLoader
participant LLM as llmgate.Client
Client->>Handler: POST /v1/answer/store
Handler->>Handler: validate config, truncate max_docs
Handler->>Map: mapTreeWalk(docIDs)
loop per document
Map->>Loader: LoadTree(docID)
Loader-->>Map: tree
Map->>Map: walk tree, collect docAnswer
end
Map-->>Handler: docAnswers
Handler->>Handler: filter non-answers, assign citation indices
Handler->>LLM: synthesise(contributing docAnswers)
LLM-->>Handler: reducer JSON response
Handler->>Handler: parseStoreAnswer, buildStoreCitations
Handler-->>Client: answer + citations
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/handler/answer_store.go`:
- Around line 145-150: The no-match response in answer_store’s map-stage flow is
masking complete traversal failures from LoadTree/SelectWithCost. Update the
logic around the contributing-documents path in answer_store so failures are
tracked separately from true no-match results, and if every document walk fails,
return an error instead of the 200 “does not address this query” response. Use
the existing answer assembly path and the contributing/docIDs handling to
distinguish partial skips from total failure, and apply the same fix
consistently in the related answer paths mentioned by the review.
- Around line 426-429: The negative-findings filter in isIrrelevant-like string
matching is too broad and is excluding legitimate answers that state a document
does not contain something relevant. Tighten the logic in the answer filtering
helper that checks phrases like “does not address” and “does not contain” so it
only rejects clear non-answers, not substantive negative findings. Update the
matching in the relevant string-checking function to distinguish true
irrelevance from useful statements such as absence of an arbitration clause.
- Around line 251-256: The sort in answer ordering only breaks ties by
confidence and docTitle, so equal values can still produce nondeterministic
results. Update the comparator in the stable sort used by the answer_store
ordering logic to add a final tie-breaker on docID after docTitle, keeping the
existing confidence-first and title-second behavior in the same function.
- Around line 389-392: The fallback in parseStoreAnswer is too aggressive
because it expands to every document whenever cited is empty, even when the
answer text already contains usable [n] markers. Update parseStoreAnswer to
first extract citation markers from the answer field itself, then use those
parsed indices before falling back to all docs; keep the existing docs loop only
for cases where no markers can be found at all. Use the parseStoreAnswer logic
and its cited/answer handling to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e63b45c7-b9d6-46a3-ac81-e5295afc0ce6
📒 Files selected for processing (3)
internal/handler/answer_store.gointernal/handler/answer_store_test.gointernal/handler/router.go
Comment on lines
+145
to
+150
| if len(contributing) == 0 { | ||
| writeJSON(w, http.StatusOK, map[string]any{ | ||
| "query": body.Query, | ||
| "answer": "The documents in this store do not appear to address this query.", | ||
| "answer": "The documents in this collection do not appear to address this query.", | ||
| "citations": []any{}, | ||
| "documents_searched": len(body.DocumentIDs), | ||
| "documents_searched": len(docIDs), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t convert map-stage failures into a no-match answer.
LoadTree/SelectWithCost errors are logged and skipped, so if every document fails, Line 145 returns a 200 saying the collection does not address the query. Track failures separately and return an error when no document was successfully walked.
Proposed direction
- perDoc, mapUsage := h.mapTreeWalk(r.Context(), orgID, storeID(r), docIDs, body.Query, model, body.MaxDepth)
+ perDoc, mapUsage, mapFailures := h.mapTreeWalk(r.Context(), orgID, storeID(r), docIDs, body.Query, model, body.MaxDepth)
+ if len(perDoc) == 0 && mapFailures > 0 {
+ writeErr(w, http.StatusBadGateway, "answer map failed for all documents")
+ return
+ }-func (h *AnswerStoreHandler) mapTreeWalk(ctx context.Context, orgID, storeID string, docIDs []tree.DocumentID, query, model string, maxDepth int) ([]docAnswer, retrieval.Usage) {
+func (h *AnswerStoreHandler) mapTreeWalk(ctx context.Context, orgID, storeID string, docIDs []tree.DocumentID, query, model string, maxDepth int) ([]docAnswer, retrieval.Usage, int) {
var (
- mu sync.Mutex
- out = make([]docAnswer, 0, len(docIDs))
- usage retrieval.Usage
+ mu sync.Mutex
+ out = make([]docAnswer, 0, len(docIDs))
+ usage retrieval.Usage
+ failures int
)
+ recordFailure := func() {
+ mu.Lock()
+ failures++
+ mu.Unlock()
+ }
...
if err != nil || t == nil {
h.logger.Warn("answer/store: load tree", "doc", docID, "err", err)
+ recordFailure()
return nil
}
...
if err != nil {
h.logger.Warn("answer/store: treewalk", "doc", docID, "err", err)
+ recordFailure()
return nil
}
...
- return out, usage
+ return out, usage, failures
}Also applies to: 195-210, 249-249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/answer_store.go` around lines 145 - 150, The no-match
response in answer_store’s map-stage flow is masking complete traversal failures
from LoadTree/SelectWithCost. Update the logic around the contributing-documents
path in answer_store so failures are tracked separately from true no-match
results, and if every document walk fails, return an error instead of the 200
“does not address this query” response. Use the existing answer assembly path
and the contributing/docIDs handling to distinguish partial skips from total
failure, and apply the same fix consistently in the related answer paths
mentioned by the review.
Comment on lines
+251
to
+256
| // Deterministic order: highest confidence first, then title. | ||
| sort.SliceStable(out, func(i, j int) bool { | ||
| if out[i].confidence != out[j].confidence { | ||
| return out[i].confidence > out[j].confidence | ||
| } | ||
| return out[i].docTitle < out[j].docTitle |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a stable final tie-breaker.
Concurrent append order is nondeterministic, so equal confidence and equal title still produce nondeterministic citation indices. Tie-break by docID after title.
Proposed fix
sort.SliceStable(out, func(i, j int) bool {
if out[i].confidence != out[j].confidence {
return out[i].confidence > out[j].confidence
}
- return out[i].docTitle < out[j].docTitle
+ if out[i].docTitle != out[j].docTitle {
+ return out[i].docTitle < out[j].docTitle
+ }
+ return string(out[i].docID) < string(out[j].docID)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Deterministic order: highest confidence first, then title. | |
| sort.SliceStable(out, func(i, j int) bool { | |
| if out[i].confidence != out[j].confidence { | |
| return out[i].confidence > out[j].confidence | |
| } | |
| return out[i].docTitle < out[j].docTitle | |
| sort.SliceStable(out, func(i, j int) bool { | |
| if out[i].confidence != out[j].confidence { | |
| return out[i].confidence > out[j].confidence | |
| } | |
| if out[i].docTitle != out[j].docTitle { | |
| return out[i].docTitle < out[j].docTitle | |
| } | |
| return string(out[i].docID) < string(out[j].docID) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/answer_store.go` around lines 251 - 256, The sort in answer
ordering only breaks ties by confidence and docTitle, so equal values can still
produce nondeterministic results. Update the comparator in the stable sort used
by the answer_store ordering logic to add a final tie-breaker on docID after
docTitle, keeping the existing confidence-first and title-second behavior in the
same function.
Comment on lines
389
to
392
| if len(cited) == 0 { // model cited nothing parseable → show all sources | ||
| for _, d := range docs { | ||
| cited = append(cited, d.index) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Scrape answer markers before falling back to every document.
If the reducer returns JSON like {"answer":"... [2]","cited":[]}, this fallback emits citations for all contributing docs instead of the marker actually used. Prefer [n] markers from the answer, then fall back to all only when no markers are present.
Proposed fix in parseStoreAnswer
var parsed storeAnswerJSON
if err := json.Unmarshal([]byte(s[i:j+1]), &parsed); err == nil && strings.TrimSpace(parsed.Answer) != "" {
- return strings.TrimSpace(parsed.Answer), dedupeInRange(parsed.Cited, maxIdx)
+ answer := strings.TrimSpace(parsed.Answer)
+ cited := dedupeInRange(parsed.Cited, maxIdx)
+ if len(cited) == 0 {
+ cited = scrapeMarkers(answer, maxIdx)
+ }
+ return answer, cited
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(cited) == 0 { // model cited nothing parseable → show all sources | |
| for _, d := range docs { | |
| cited = append(cited, d.index) | |
| } | |
| var parsed storeAnswerJSON | |
| if err := json.Unmarshal([]byte(s[i:j+1]), &parsed); err == nil && strings.TrimSpace(parsed.Answer) != "" { | |
| answer := strings.TrimSpace(parsed.Answer) | |
| cited := dedupeInRange(parsed.Cited, maxIdx) | |
| if len(cited) == 0 { | |
| cited = scrapeMarkers(answer, maxIdx) | |
| } | |
| return answer, cited | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/answer_store.go` around lines 389 - 392, The fallback in
parseStoreAnswer is too aggressive because it expands to every document whenever
cited is empty, even when the answer text already contains usable [n] markers.
Update parseStoreAnswer to first extract citation markers from the answer field
itself, then use those parsed indices before falling back to all docs; keep the
existing docs loop only for cases where no markers can be found at all. Use the
parseStoreAnswer logic and its cited/answer handling to locate the change.
Rework the store map to the Vectorless primitive the collection query
should use — reason over each document's llms.txt-style structure +
summaries, select the relevant sections, pull their FULL content, then
generate:
1. per document (parallel): load the tree, render its section outline
(title + one-line summary per section), and ask the LLM which
sections' full text are relevant — reasoning over summaries, not raw
pages, and not chunks.
2. fetch the FULL content of every selected section (a selected heading
pulls its subsection's leaves).
3. one generation call over the full content of ALL relevant sections
→ a single answer citing sections + documents with [n] markers.
Two cheap selection calls over compact summaries + one generation call,
instead of N page-based tree-walks. No chunking, no embeddings, no
truncated snippets — full section content drives the answer.
Controls: max_docs (0 = all), max_sections (0 = all relevant), plus a
total content budget so a large selection still fits the model context.
Citations are per section, carrying document + section title + page span
+ a quote. Returns 501 without an LLM.
hallelx2
changed the title
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.
What
Makes the store answer (
/v1/answer/store) use the true Vectorless retrieval primitive over a collection: reason over structure + summaries → select relevant sections → pull their full content → generate. No chunking, no embeddings, no page-walking, no truncated snippets.Flow
[n]markers.That's two cheap selection calls over compact summaries + one generation call — far cheaper than N page-based tree-walks, and faithful to "reason over structure, read the relevant sections in full."
Controls
max_docs(0 = all documents)max_sections(0 = all relevant sections)Citations
Per section:
{index, document_id, document_title, section_title, section_ids, start_page, end_page, quote}— the dashboard groups them by document. Returns 501 without an LLM.Tests
TestParseStoreAnswer,TestParseRelevant,TestBuildStoreCitations,TestFormatPageRange.go build ./...+go test ./...green.History
Supersedes the earlier tree-walk-per-document map on this branch — same endpoint, the reasoning substrate moved from raw pages to structure+summaries per review discussion.