Fix single-source chat missing full document content - #1209
Fix single-source chat missing full document content#1209mike-foucault wants to merge 3 commits into
Conversation
build_source_context() (used by per-source chat/ask) requested 'short' context from Source.get_context(), which never includes full_text. A source without a transformation/insight left the LLM with only the title, making single-document chat silently useless. - Use context_size="long" so full_text is always included. - Fix the token-budget truncation loop to never drop the source itself when it alone exceeds max_tokens (previously it could, leaving an empty context for large documents). Only insights are trimmed; the prompt formatter already caps full_text length.
There was a problem hiding this comment.
Pull request overview
Fixes source-level chat context so the LLM receives the actual document content even when no transformations/insights exist, and prevents the source from being dropped when token truncation is applied.
Changes:
- Switch
build_source_context()to requestcontext_size="long"sofull_textis included. - Adjust truncation logic to drop insights to meet a token budget while keeping the source.
- Update/add tests to validate long-context usage and the “oversized source” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
open_notebook/utils/context_builder.py |
Uses long source context and changes token-budget truncation behavior for source-chat context building. |
tests/test_utils.py |
Updates characterization tests and adds coverage for oversized-source behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| total_tokens = sum(item_tokens) | ||
| if max_tokens: | ||
| while total_tokens > max_tokens and item_tokens: | ||
| while total_tokens > max_tokens and insights: | ||
| total_tokens -= item_tokens.pop() | ||
| if insights: | ||
| insights.pop() | ||
| else: | ||
| sources.pop() | ||
| insights.pop() |
| @pytest.mark.asyncio | ||
| async def test_keeps_source_when_it_alone_exceeds_budget(self): | ||
| """A source whose full_text alone exceeds max_tokens is still | ||
| returned (not dropped) — an empty context is worse than one over | ||
| budget, and the prompt formatter caps full_text length downstream.""" | ||
| source = SimpleNamespace(id="source:123") | ||
| big_text = "word " * 20000 # far bigger than the token budget below | ||
| source.get_context = AsyncMock( | ||
| return_value={"id": "source:123", "title": "T", "full_text": big_text} | ||
| ) | ||
| source.get_insights = AsyncMock(return_value=[]) | ||
|
|
||
| with patch( | ||
| "open_notebook.utils.context_builder.Source.get", | ||
| new=AsyncMock(return_value=source), | ||
| ): | ||
| result = await build_source_context("source:123", max_tokens=600) | ||
|
|
||
| assert len(result["sources"]) == 1 | ||
| assert result["sources"][0]["full_text"] == big_text | ||
| assert result["total_tokens"] > 600 | ||
|
|
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Addresses PR lfnovo#1209 review comment: dropping insights when the source alone already exceeds max_tokens can't bring the total under budget, so it was discarding context for no benefit. Also switch the max_tokens check to 'is not None' so max_tokens=0 isn't silently treated the same as no limit.
| if source: | ||
| source_context = await source.get_context(context_size="short") | ||
| source_context = await source.get_context(context_size="long") | ||
| sources.append(source_context) | ||
| item_tokens.append(token_count(str(source_context))) |
| # Long context is used so full_text is always included — a source | ||
| # with no insights would otherwise leave the LLM with only the title. | ||
| source.get_context.assert_awaited_once_with(context_size="long") |
Source.get_context() fetches and embeds insights itself when not given a list, so build_source_context() (which fetches and represents insights separately) was hitting the DB twice and double-counting insight tokens toward max_tokens. Pass insights=[] so get_context() leaves insight representation to the caller.
| source_context = await source.get_context( | ||
| context_size="long", insights=[] | ||
| ) | ||
| sources.append(source_context) | ||
| item_tokens.append(token_count(str(source_context))) |
| Used by the source-chat graph. Uses the "long" context so the source's | ||
| full text is always included — a source with no insights/transformations | ||
| would otherwise leave the LLM with only the title. If `max_tokens` is | ||
| given, insights are dropped (last-fetched first) until the total fits. | ||
| The source itself is never dropped — a large full_text can alone exceed | ||
| the budget, and dropping it would leave an empty context; the caller | ||
| (source-chat's prompt formatter) already caps full_text at a fixed size | ||
| before it reaches the LLM. |
Problem
Chatting/asking questions about an individual source (not a notebook) silently failed to use the document's actual content when no transformation/insight had been generated for it — the LLM only ever saw the source's title, with no error or warning. Users would get generic "I have no document to analyze" style answers without understanding why.
Root cause
build_source_context()(inopen_notebook/utils/context_builder.py, used by the source-chat graph) requestedcontext_size="short"fromSource.get_context(). That mode never includesfull_text— onlyid,title,insights. Without insights (i.e. no transformation run on the source), the context was reduced to just the title.Switching to
context_size="long"alone was not enough: it exposed a second, previously latent bug. The function's token-budget truncation loop dropped the entire source whenever itsfull_textalone exceededmax_tokens(very common for real documents, since the budget is 50k tokens) — leaving an empty context for exactly the documents most likely to have real content.Fix
context_size="long"inbuild_source_contextsofull_textis always included, regardless of whether the source has insights._format_source_contextinopen_notebook/graphs/source_chat.py) already capsfull_textat a fixed size before it reaches the LLM, so this doesn't risk oversized prompts.Tests
Added/updated tests in
tests/test_utils.py:test_source_and_insights_shapenow assertscontext_size="long"is requested.test_keeps_source_when_it_alone_exceeds_budget: a source whosefull_textalone exceedsmax_tokensis still returned instead of being dropped.test_truncates_insights_to_token_budgetandtest_missing_source_yields_empty_contextstill pass unchanged in behavior.Also manually verified end-to-end locally: uploaded a document with no transformation, opened its chat, and confirmed the LLM's response now reflects the document's actual content instead of the "no document" fallback.
This is a small, self-contained bug fix (per CONTRIBUTING.md — "small, obvious bug fixes" are exempt from requiring a prior Discussion/Issue).