fix(compiler): cap concept/entity brief lists in the plan prompt - #268
Closed
rodrigedilson-ia wants to merge 1 commit into
Closed
rodrigedilson-ia wants to merge 1 commit into
rodrigedilson-ia wants to merge 1 commit into
Conversation
`_read_concept_briefs` and `_read_entity_briefs` read every page in the KB and the result is rebuilt into the concepts-plan prompt for each compiled document. Their size is therefore O(KB), with no ceiling — so a large KB eventually pushes that call past the model's context window. The failure mode is unkind: it lands as a hard error partway through a recompile, not as degraded output. We hit it on a KB of ~260 documents, where seven consecutive `recompile` runs exited non-zero, all of them at the plan step, with prompts measured between 200k and 356k tokens against a 200k limit. Because it scales with the KB, every retry was guaranteed to fail the same way. This adds a per-list character budget (`briefs_budget_chars`, default 120_000 — roughly 30k tokens each, so a KB has to grow well past a few hundred pages before anything is trimmed). Set it to 0 to restore the previous uncapped behavior. Two details worth calling out: - **Ranking is by source count, not alphabetical.** That count is already the cross-document recurrence signal the plan call uses for create-vs-update, so it is also the right thing to keep when the list has to be cut. Ties break alphabetically, because prompt caching depends on byte-identical prefixes and an unstable order would silently cost cache hits. - **The trim is announced in the list.** A shortened list that reads as complete is worse than a long one: the planner uses these briefs to decide create-vs-update, so a silently dropped concept comes back as a duplicate page for something the KB already has. The trailing marker states how many were omitted and tells the model to prefer `update` when unsure. In the degenerate case where the budget cannot fit even one line, the output says the list was omitted rather than returning an empty one — an empty list reads as an empty KB, which would push the planner to recreate everything. Small KBs are byte-identical to before (covered by a test), so prompt caches are not invalidated for existing users.
Author
|
Closing this — we're handling it on our side instead of upstreaming, so I don't want to leave a PR open that I won't follow through on. Apologies for the noise. Leaving the finding here in case it's useful to anyone: The branch stays on my fork if anyone wants to pick it up. |
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
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.
Problem
_read_concept_briefsand_read_entity_briefsread every page in the KB, and the result is rebuilt into the concepts-plan prompt for each compiled document. Their size is therefore O(KB), with no ceiling — so a sufficiently large KB pushes that call past the model's context window.The failure mode is unkind: it lands as a hard error partway through a recompile, not as degraded output. And because it scales with the KB rather than with the document, every retry is guaranteed to fail the same way.
We hit this on a KB of ~260 documents: seven consecutive
recompileruns exited non-zero, all of them at the plan step, with prompts measured between 200k and 356k tokens against a 200k limit.Change
A per-list character budget,
briefs_budget_chars, default120_000(roughly 30k tokens each) — a KB has to grow well past a few hundred pages before anything is trimmed. Set it to0to restore the previous uncapped behavior.Two details worth review attention:
Ranking is by source count, not alphabetical. That count is already the cross-document recurrence signal the plan call uses for create-vs-update, so it is also the most defensible thing to keep when the list has to be cut. Ties break alphabetically — prompt caching depends on byte-identical prefixes, and an unstable order would silently cost cache hits.
The trim is announced inside the list. A shortened list that reads as complete is worse than a long one here: the planner uses these briefs to decide create-vs-update, so a silently dropped concept comes back as a duplicate page for something the KB already has. The trailing marker states how many were omitted and tells the model to prefer
updatewhen unsure.In the degenerate case where the budget cannot fit even one line, the output says the list was omitted rather than returning an empty one — an empty list reads as an empty KB, which would push the planner to recreate everything.
Compatibility
Small KBs produce byte-identical output to before (covered by
test_under_budget_is_unchanged), so existing prompt caches are not invalidated.Verification
New tests in
tests/test_compiler.py:TestBriefsBudget(under-budget identity, salience ranking, announced truncation,0disables, degenerate budget, stable ordering, entities) andTestResolveBriefsBudget(config fallback on a typo — a bad value should not abort a compile).The four
tests/test_api*.pymodules were not run locally: they need the optionalfastapiextra, and they are untouched by this change.