Optimize hook injection fallback and filtering#232
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR enhances the prompt search mechanism with four features: low-confidence fallback messaging when search relevance scores fall below a threshold, improved snippet truncation at sentence boundaries using regex detection, schema-aware filtering of KG entity chunks to exclude co-occurrence relations, and corresponding test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SearchEngine
participant Database
participant RelationFilter
participant Truncator
participant FallbackHandler
Client->>SearchEngine: Initiate search
SearchEngine->>Database: Retrieve candidate rows with RRF scores
Database-->>SearchEngine: Return filtered rows
SearchEngine->>RelationFilter: Check KG entity chunks for relation_type
RelationFilter->>Database: Query schema for relation_type column
Database-->>RelationFilter: Schema info
RelationFilter-->>SearchEngine: Exclude co_occurs_with relations
SearchEngine->>Truncator: Build snippets with truncation
Truncator->>Truncator: Find last sentence boundary [.!?]
Truncator-->>SearchEngine: Return truncated snippets
SearchEngine->>FallbackHandler: Check if context lines produced
alt No context lines & low confidence
FallbackHandler->>FallbackHandler: Check RRF score vs 0.30 threshold
FallbackHandler-->>SearchEngine: Return fallback message
else Score above threshold or lines produced
FallbackHandler-->>SearchEngine: Return None
end
SearchEngine-->>Client: Return final results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@codex review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/brainlayer-prompt-search.py`:
- Around line 1244-1248: The current logic unconditionally calls
build_low_confidence_fallback(fallback_rows) when lines is empty, but
fallback_rows is only populated for the FTS paths (knowledge_question,
follow_up, hebrew_query) and not for entity_lookup; change the condition so the
low-confidence fallback is only built/added when fallback_rows is non-empty or
when the classification is one of the FTS branches (e.g., knowledge_question,
follow_up, hebrew_query), and for entity_lookup either skip building that
fallback or implement a separate entity-specific fallback; update the block
referencing fallback_rows, build_low_confidence_fallback, and the entity_lookup
classification to guard the fallback call accordingly.
- Around line 737-753: The fallback_rows creation currently hardcodes
rrf_score=0.0 which makes build_low_confidence_fallback() always treat the top
row as low-confidence; either compute a real relevance/rrf score for those
synthetic rows or stop setting rrf_score at all so
build_low_confidence_fallback() won't see a numeric score and won't auto-trigger
fallback. Update the code that builds fallback_rows (the block that sets
"rrf_score: 0.0") to either calculate a proper relevance value or omit/set
rrf_score to None, and keep
build_low_confidence_fallback(top_row.get("relevance") /
top_row.get("rrf_score")) logic unchanged.
- Around line 434-448: The function _kg_entity_chunks_has_relation_type
currently defensively checks for a future relation_type column and caches the
result in _KG_ENTITY_CHUNKS_RELATION_TYPE_CACHE; update the codebase by either
(A) adding a brief docstring above _kg_entity_chunks_has_relation_type
explaining this is forward-compatible support for an expected relation_type
column (and note where/when it will be added, e.g., vector_store.py schema
changes), or (B) if relation_type is not planned, remove the check and related
test usage and update test_prompt_classification to match production schema;
reference the function name _kg_entity_chunks_has_relation_type and the cache
symbol _KG_ENTITY_CHUNKS_RELATION_TYPE_CACHE when making the change so reviewers
can locate and verify the update.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45d2285c-7134-4dd0-a008-dafa5a534392
📒 Files selected for processing (4)
hooks/brainlayer-prompt-search.pytests/test_adaptive_injection.pytests/test_hook_slim.pytests/test_prompt_classification.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Flag risky DB or concurrency changes explicitly and do not hand-wave lock behavior
Enforce one-write-at-a-time concurrency constraint; reads are safe but brain_digest is write-heavy and must not run in parallel with other MCP work
Run pytest before claiming behavior changed safely; current test suite has 929 tests
**/*.py: Usepaths.py:get_db_path()for all database path resolution; all scripts and CLI must use this function rather than hardcoding paths
When performing bulk database operations: stop enrichment workers first, checkpoint WAL before and after, drop FTS triggers before bulk deletes, batch deletes in 5-10K chunks, and checkpoint every 3 batches
Files:
tests/test_adaptive_injection.pytests/test_hook_slim.pytests/test_prompt_classification.pyhooks/brainlayer-prompt-search.py
🧠 Learnings (3)
📚 Learning: 2026-04-04T15:21:39.570Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 198
File: hooks/brainlayer-prompt-search.py:169-169
Timestamp: 2026-04-04T15:21:39.570Z
Learning: In EtanHey/brainlayer, `hooks/brainlayer-prompt-search.py` reads `entity_type` directly from existing rows in `kg_entities` (read-only). `contracts/entity-types.yaml` defines the write-side schema only and is not authoritative for what `entity_type` values exist in the DB. The DB already stores `technology` (72 entities), `project` (24), and `tool` (1) as valid `entity_type` values, so `INJECT_TYPES` in the hook should match these DB values, not the contract file.
Applied to files:
tests/test_prompt_classification.py
📚 Learning: 2026-04-04T15:22:02.740Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 198
File: hooks/brainlayer-prompt-search.py:241-259
Timestamp: 2026-04-04T15:22:02.740Z
Learning: In `hooks/brainlayer-prompt-search.py` (Python), `record_injection_event()` is explicitly best-effort telemetry: silent `except sqlite3.Error: pass` is intentional — table non-existence or lock failures are acceptable silent failures. `sqlite3.connect(timeout=2)` is the file-open timeout; `PRAGMA busy_timeout` governs per-statement lock-wait. The `DEADLINE_MS` (450ms) guard applies only to the FTS search phase, not to this side-channel write.
Applied to files:
hooks/brainlayer-prompt-search.py
📚 Learning: 2026-04-06T08:40:13.531Z
Learnt from: CR
Repo: EtanHey/brainlayer PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T08:40:13.531Z
Learning: Applies to src/brainlayer/**/*.py : Implement chunk lifecycle columns: `superseded_by`, `aggregated_into`, `archived_at` on chunks table; exclude lifecycle-managed chunks from default search; allow `include_archived=True` to show history
Applied to files:
hooks/brainlayer-prompt-search.py
🔇 Additional comments (9)
hooks/brainlayer-prompt-search.py (3)
43-47: LGTM: Well-defined constants for fallback behavior.The threshold of 0.30 for low-confidence fallback is clearly documented, and the message appropriately guides users to use
brain_search()for deeper retrieval. The module-level cache for relation_type column existence is a reasonable optimization.
402-411: LGTM: Improved sentence-boundary truncation.The regex-based approach
[.!?](?=(?:\s|$))correctly identifies sentence-ending punctuation followed by whitespace or end-of-string. Usingfinditerto find the last match before the limit ensures clean truncation at natural boundaries. Falls back to word-boundary split if no sentence end is found.
640-663: LGTM: Safe conditional SQL construction.The f-string usage here is acceptable since
relation_filteris a hardcoded constant, not user input. TheCOALESCE(ec.relation_type, '') != 'co_occurs_with'correctly handles NULL values, ensuring rows without a relation_type are included.tests/test_adaptive_injection.py (1)
157-175: LGTM: Comprehensive tests for fallback behavior.The three test cases properly cover:
- Empty input → fallback message
- Below-threshold score (0.29) → fallback message
- At-threshold score (0.30) → no fallback (returns None)
The tests correctly exercise the boundary at 0.30 and validate the return type contract.
tests/test_prompt_classification.py (4)
38-47: LGTM: Test schema includesrelation_typeto validate filtering.The test database schema intentionally includes the
relation_typecolumn that doesn't exist in the current production schema. This correctly tests the forward-compatible filtering behavior introduced inget_entity_chunks().
94-111: Effective test for co-occurrence filtering.The test data is well-designed:
chunk-theo-noisehas higher relevance (0.99) than the legitimate chunk (0.9), ensuring the test validates that filtering byrelation_type='co_occurs_with'works correctly rather than just relying on ordering.
165-179: LGTM: Skip tests correctly isolate DB access.Patching
get_db_pathto returnNoneensuresfinalize_and_exit(mode="skip")is called before any database connection attempt, correctly validating that command and casual prompts bypass retrieval entirely.
197-199: LGTM: Assertion validates co-occurrence filtering in entity route.The added assertion
assert "co-occurrence noise" not in outputdirectly validates that theco_occurs_withfiltering excludes the noise chunk despite its higher relevance score.tests/test_hook_slim.py (1)
58-66: LGTM: Test validates sentence-boundary truncation.This test correctly validates the new truncation behavior:
- Input has three sentences with the third extending past the 55-char limit
- The function correctly truncates at the last sentence boundary (position 42) before the limit
- Output includes the trailing "..." resulting in the expected "...sentence...." (4 dots total)
| def _kg_entity_chunks_has_relation_type(conn): | ||
| cache_key = _get_connection_cache_key(conn) | ||
| cached = _KG_ENTITY_CHUNKS_RELATION_TYPE_CACHE.get(cache_key) | ||
| if cached is not None: | ||
| return cached | ||
|
|
||
| try: | ||
| columns = {row[1] for row in conn.execute("PRAGMA table_info(kg_entity_chunks)").fetchall()} | ||
| except sqlite3.Error: | ||
| columns = set() | ||
|
|
||
| has_column = "relation_type" in columns | ||
| _KG_ENTITY_CHUNKS_RELATION_TYPE_CACHE[cache_key] = has_column | ||
| return has_column | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if relation_type column exists in kg_entity_chunks schema definition
rg -n "kg_entity_chunks" --type py -A 10 | head -60
# Check if there's a migration adding relation_type to kg_entity_chunks
rg -n "relation_type" --type py -C 3 | grep -i "kg_entity_chunks" | head -20Repository: EtanHey/brainlayer
Length of output: 4918
🏁 Script executed:
# Find and examine the actual kg_entity_chunks schema definition in vector_store.py
rg -n "CREATE TABLE kg_entity_chunks" --type py -A 20
# Also check for any ALTER TABLE statements that might add relation_type
rg -n "ALTER TABLE kg_entity_chunks" --type py -A 5Repository: EtanHey/brainlayer
Length of output: 4124
🏁 Script executed:
# Find the initial CREATE TABLE for kg_entity_chunks in vector_store.py
rg -n "CREATE TABLE kg_entity_chunks" src/brainlayer/vector_store.py -A 10
# Also search broadly for relation_type in production code (not tests)
rg -n "relation_type" src/brainlayer/ --type pyRepository: EtanHey/brainlayer
Length of output: 8273
🏁 Script executed:
# Look around the ALTER TABLE statements to find initial CREATE TABLE
sed -n '600,900p' src/brainlayer/vector_store.py | cat -n | grep -E "(CREATE TABLE|kg_entity_chunks|ALTER TABLE)" -A 3 -B 3
# Also try direct search with broader context
rg -n "CREATE TABLE" src/brainlayer/vector_store.py | grep -i entityRepository: EtanHey/brainlayer
Length of output: 6993
🏁 Script executed:
# Check how _kg_entity_chunks_has_relation_type is used in the hook
rg -n "_kg_entity_chunks_has_relation_type" hooks/brainlayer-prompt-search.py -B 2 -A 8
# Look at line 644 context mentioned in initial results
sed -n '640,660p' hooks/brainlayer-prompt-search.pyRepository: EtanHey/brainlayer
Length of output: 1691
🏁 Script executed:
# Search for TODOs, FIXMEs, or comments about relation_type and kg_entity_chunks
rg -n "TODO|FIXME|XXX" hooks/brainlayer-prompt-search.py | head -20
# Check if there are any schema migration files or pending work
find . -name "*migrat*" -type f 2>/dev/null | head -10
# Search for any comments mentioning this forward compatibility
rg -n "relation_type.*kg_entity_chunks|kg_entity_chunks.*relation_type" --type py -B 3 -A 3Repository: EtanHey/brainlayer
Length of output: 2170
🏁 Script executed:
# Check the migrate.py file for relation_type migration
cat src/brainlayer/migrate.py | head -100
# Also search for any migration or TODO comments about this
rg -n "relation_type" src/brainlayer/migrate.py -B 3 -A 3Repository: EtanHey/brainlayer
Length of output: 3299
🏁 Script executed:
# Search for any schema evolution or chunk lifecycle plans
rg -n "relation_type|lifecycle|archived_at" src/brainlayer/ --type py | grep -E "(mention_type|archived|superseded)" | head -20
# Check if relation_type filtering is actually needed by looking at where co_occurs_with is used
rg -n "co_occurs_with" src/brainlayer/ --type py | head -15Repository: EtanHey/brainlayer
Length of output: 2885
Document forward-compatible schema detection or clarify the intended timeline for relation_type in kg_entity_chunks.
The defensive check is sound—the relation_type column doesn't exist in kg_entity_chunks in production (created at vector_store.py:81-89 with only entity_id, chunk_id, relevance), and co_occurs_with filtering happens at the kg_relations level, not here. The function gracefully handles this by returning an empty filter string when the column is absent, so no filtering occurs in production.
However, the test schema in test_prompt_classification.py:40-45 includes relation_type in kg_entity_chunks, suggesting this may be planned. Either document this as intentional forward-compatibility or clarify whether and when relation_type will be added to the production schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/brainlayer-prompt-search.py` around lines 434 - 448, The function
_kg_entity_chunks_has_relation_type currently defensively checks for a future
relation_type column and caches the result in
_KG_ENTITY_CHUNKS_RELATION_TYPE_CACHE; update the codebase by either (A) adding
a brief docstring above _kg_entity_chunks_has_relation_type explaining this is
forward-compatible support for an expected relation_type column (and note
where/when it will be added, e.g., vector_store.py schema changes), or (B) if
relation_type is not planned, remove the check and related test usage and update
test_prompt_classification to match production schema; reference the function
name _kg_entity_chunks_has_relation_type and the cache symbol
_KG_ENTITY_CHUNKS_RELATION_TYPE_CACHE when making the change so reviewers can
locate and verify the update.
| def build_low_confidence_fallback(rows): | ||
| if not rows: | ||
| return LOW_CONFIDENCE_FALLBACK_MESSAGE | ||
|
|
||
| top_row = rows[0] | ||
| if isinstance(top_row, dict): | ||
| relevance = top_row.get("relevance") | ||
| if relevance is None: | ||
| relevance = top_row.get("rrf_score") | ||
| else: | ||
| relevance = None | ||
|
|
||
| if relevance is not None and relevance < LOW_CONFIDENCE_FALLBACK_THRESHOLD: | ||
| return LOW_CONFIDENCE_FALLBACK_MESSAGE | ||
|
|
||
| return None | ||
|
|
There was a problem hiding this comment.
Bug: fallback_rows always have rrf_score=0.0, causing fallback to always trigger.
The fallback_rows constructed at lines 1218-1229 hardcode rrf_score: 0.0 for all rows. When build_low_confidence_fallback() checks the top row's relevance/rrf_score, it will always find 0.0 < 0.30, causing the fallback message to always be emitted when not lines is true.
If the intent is to use the actual FTS rank or relevance score, you need to compute a meaningful score. If the intent is to always show the fallback when no search results were injected, the threshold check is unnecessary.
🔧 Possible fix if intent is to always emit fallback when no lines
def build_low_confidence_fallback(rows):
if not rows:
return LOW_CONFIDENCE_FALLBACK_MESSAGE
-
- top_row = rows[0]
- if isinstance(top_row, dict):
- relevance = top_row.get("relevance")
- if relevance is None:
- relevance = top_row.get("rrf_score")
- else:
- relevance = None
-
- if relevance is not None and relevance < LOW_CONFIDENCE_FALLBACK_THRESHOLD:
- return LOW_CONFIDENCE_FALLBACK_MESSAGE
-
- return None
+ # Always return fallback when called (caller determines when to call)
+ return LOW_CONFIDENCE_FALLBACK_MESSAGEAlternatively, compute actual relevance scores in fallback_rows instead of hardcoding 0.0.
Also applies to: 1218-1229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/brainlayer-prompt-search.py` around lines 737 - 753, The fallback_rows
creation currently hardcodes rrf_score=0.0 which makes
build_low_confidence_fallback() always treat the top row as low-confidence;
either compute a real relevance/rrf score for those synthetic rows or stop
setting rrf_score at all so build_low_confidence_fallback() won't see a numeric
score and won't auto-trigger fallback. Update the code that builds fallback_rows
(the block that sets "rrf_score: 0.0") to either calculate a proper relevance
value or omit/set rrf_score to None, and keep
build_low_confidence_fallback(top_row.get("relevance") /
top_row.get("rrf_score")) logic unchanged.
| if not lines: | ||
| fallback = build_low_confidence_fallback(fallback_rows) | ||
| if fallback: | ||
| lines.append(fallback) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Logic concern: Fallback only applies to FTS-based paths, not entity lookup.
The fallback_rows variable is only populated in the knowledge_question, follow_up, and hebrew_query branches. For entity_lookup classification, fallback_rows remains empty, so build_low_confidence_fallback([]) would return the fallback message even if entity cards were successfully injected but lines was empty for other reasons.
Consider whether this is the intended behavior or if entity lookup should have its own fallback handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/brainlayer-prompt-search.py` around lines 1244 - 1248, The current
logic unconditionally calls build_low_confidence_fallback(fallback_rows) when
lines is empty, but fallback_rows is only populated for the FTS paths
(knowledge_question, follow_up, hebrew_query) and not for entity_lookup; change
the condition so the low-confidence fallback is only built/added when
fallback_rows is non-empty or when the classification is one of the FTS branches
(e.g., knowledge_question, follow_up, hebrew_query), and for entity_lookup
either skip building that fallback or implement a separate entity-specific
fallback; update the block referencing fallback_rows,
build_low_confidence_fallback, and the entity_lookup classification to guard the
fallback call accordingly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 450f654bd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not lines: | ||
| fallback = build_low_confidence_fallback(fallback_rows) |
There was a problem hiding this comment.
In main(), the low-confidence message is gated by if not lines, but lines is a presentation buffer rather than a retrieval signal. After dedup, filtered_rows can be empty even when the original query returned relevant chunks that were already injected, so this branch emits “No high-confidence memories found” for repeated/follow-up prompts where memories do exist. That is misleading and can spam fallback guidance in active sessions; the condition should be tied to actual search outcome/new-memory availability rather than whether any text was appended to lines.
Useful? React with 👍 / 👎.
|
@coderabbitai Learning: co_occurs_with is filtered at SQL-time because it represents 85.5% of KG relations and is same-chunk co-occurrence noise, not a semantic relationship. Entity cards should surface real edges (DEPENDS_ON, WORKS_AT, MERGED_BY, etc.), not the bag-of-words baseline. Please learn this pattern for future reviews. |
|
Tip For best results, initiate chat on the files or code changes.
I'll flag any future code that inadvertently re-introduces ✏️ Learnings added
🧠 Learnings used |
1 participant
Summary
kg_entity_chunksreads to excludeco_occurs_withlinks when the column existsTest plan
pytest tests/test_hook_slim.py tests/test_follow_up_rewrite.py -qpytest tests/test_hook_slim.py tests/test_adaptive_injection.py tests/test_prompt_classification.py -qpytest tests/test_hook_slim.py tests/test_adaptive_injection.py tests/test_prompt_classification.py tests/test_follow_up_rewrite.py -qSummary by CodeRabbit
New Features
Tests
Note
Optimize hook injection fallback and filter
co_occurs_withrelations from entity chunk retrievalbrain_search()) when search results are empty or all scores fall below a 0.30 threshold viabuild_low_confidence_fallbackin brainlayer-prompt-search.pyco_occurs_withrelation type rows fromget_entity_chunksqueries when thekg_entity_chunkstable schema includes arelation_typecolumn; schema detection is cached per-connection[.!?]) within the limit rather than splitting at an arbitrary spaceco_occurs_with-linked chunks by default when the DB schema supports itMacroscope summarized 450f654.