fix: address review findings from 3-block system prompt PR#312
Merged
Conversation
Critical: - OpenAI/Responses API upstreams now receive LTM in system prompt (previously silently dropped — only Anthropic path got LTM) - vectorSearch() now accepts excludeCategories filter, preventing preference entries from consuming vector search slots when context- bound entries are requested separately Medium: - textDiffRatio rewritten to sample across full string length instead of only prefix+suffix — catches interior content changes that the old heuristic missed - Context-bound LTM budget floor: guaranteed 50% of total LTM budget even when preferences are large, preventing preference starvation Low: - KnowledgeCategory type alias for well-known categories with autocomplete - ForSessionOptions doc clarifies categories/excludeCategories mutual exclusion - Tests for excludeCategories on cross-project entries and empty array edge case
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.
Summary
Fixes all pre-existing and newly-identified issues from the code review of PR #311 (3-block system prompt).
Critical fixes
OpenAI/Responses API upstreams now receive LTM: Previously, LTM text was only passed via
AnthropicCacheOptions, which onlybuildAnthropicRequestconsumed. OpenAI Chat Completions and Responses API paths receivedreq.systemunmodified (no LTM). Fix:forwardToUpstreamnow concatenatesstableLtmSystem+ltmSystemintoreq.systembefore calling OpenAI builders.vectorSearch()now filters by category: Previously queried all knowledge entries with embeddings regardless of category. WhenexcludeCategories: ["preference"]was passed toforSession(), the SQL filtered preferences out of the entry lists, butvectorSearch()still returned preference entries in its top-50 hits — crowding out relevant context-bound entries. Fix: add optionalexcludeCategoriesparameter tovectorSearch(), propagated fromforSession().Medium fixes
textDiffRatiorewritten: Old implementation only compared prefix + suffix characters, missing interior content changes entirely (e.g., entries added/removed in the middle). New implementation samples up to 1000 evenly-spaced positions across the full string length for O(1) cost, reliably detecting interior edits.Context-bound LTM budget floor: Added a 50% minimum floor for context-bound entries. Previously, a project with many preferences could consume the entire LTM budget, starving gotchas/patterns/architecture entries that are more critical for correctness during active work.
Low fixes
KnowledgeCategorytype alias: Exported fromltm.tsfor autocomplete on well-known categories (decision,pattern,preference,architecture,gotcha).ForSessionOptions.categoriesandexcludeCategoriesuse(KnowledgeCategory | (string & {}))[]for type safety with escape hatch.Cross-project
excludeCategoriestest: Verifies the filter applies to cross-project entries too, not just project-local ones.Empty
excludeCategoriesarray test: ConfirmsexcludeCategories: []has no filtering effect (falsy.lengthcheck).