test: Phase 4 QA — comprehensive tests for Phase 3 core fixes#4
test: Phase 4 QA — comprehensive tests for Phase 3 core fixes#4
Conversation
EtanHey
force-pushed
the
test/phase3-qa
branch
from
f0895d9 to
569097f
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| """Read-only connection to the production DB for integration tests.""" | ||
| s = VectorStore(DEFAULT_DB_PATH) | ||
| yield s | ||
| s.close() |
There was a problem hiding this comment.
Tests now run DDL against production database
High Severity
The test fixture connects to the production database via VectorStore(DEFAULT_DB_PATH). The VectorStore.__init__ calls _init_db(), which runs DDL statements (ALTER TABLE, CREATE INDEX, CREATE TRIGGER) against the production database every time tests execute. These tests also fail on any machine without the developer's specific ~/.local/share/zikaron/zikaron.db database, breaking CI/CD and other contributors' environments. The previous version used isolated tmp_path fixtures, which were safe and portable.
Additional Locations (1)
EtanHey
force-pushed
the
test/phase3-qa
branch
from
569097f to
f674738
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds comprehensive test coverage across multiple modules: a new test suite for project name normalization, database path resolution, vector store schema and metadata validation, and end-to-end QA testing. The existing vector store test suite is refactored to emphasize date filtering and metadata integrity while adopting a read-only integration test pattern using production database paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
28 new tests covering: - DB path resolution (env var, legacy, canonical) - Date filtering (date_from, date_to, ranges) in search + hybrid_search - Project name normalization (Claude Code paths, worktrees, compound names) - Search metadata (created_at, source in results) - Chunk boundary improvements (sentence-aware splitting) - created_at storage in upsert_chunks Full suite: 160 passed, 2 skipped, 0 failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- test_paths.py: DB path resolution order (env, legacy, canonical) - test_normalize_project.py: Claude Code path encoding, worktree stripping - test_chunker.py: sentence-aware splitting, content-type filtering - test_vector_store.py: schema verification, date filtering, search metadata Unit tests: 19/19 pass (paths, normalization, chunker) Integration tests: 3/3 schema tests pass, search tests need embedding model Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
EtanHey
force-pushed
the
test/phase3-qa
branch
from
41f0d06 to
ea52424
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/test_normalize_project.pytests/test_paths.pytests/test_phase3_qa.pytests/test_vector_store.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_paths.py (2)
src/brainlayer/paths.py (1)
get_db_path(23-39)tests/test_phase3_qa.py (1)
test_env_var_override(31-35)
tests/test_normalize_project.py (2)
src/brainlayer/mcp/__init__.py (1)
normalize_project_name(22-76)tests/test_phase3_qa.py (1)
test_worktree_suffix_stripped(213-217)
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_normalize_project.py`:
- Around line 11-63: The TestNormalizeProjectName test class duplicates many
cases already covered by TestProjectNameNormalization; consolidate by removing
the redundant tests (or the entire TestNormalizeProjectName class) and keep a
single canonical test suite (e.g., TestProjectNameNormalization) that includes
the unique cases; locate the duplicated test methods by name
(test_none_returns_none / test_none_input, test_empty_returns_none /
test_empty_string|test_dash_only|test_whitespace, test_simple_name_passthrough /
test_clean_name_passthrough, test_worktree_suffix_stripped,
test_no_gits_segment_returns_none / test_no_gits_segment) and either move any
unique assertions into the canonical class or delete them, then run the test
suite to ensure no missing imports or references to normalize_project_name
remain.
- Around line 32-37: The test.test_desktop_gits_path currently asserts only that
normalize_project_name("-Users-etanheyman-Desktop-Gits-golems") is not None;
strengthen it to verify the actual decoded/fallback value returned by
normalize_project_name is "golems" (same expectation as
test_claude_code_encoded_path). Update the assertion in test_desktop_gits_path
to compare the result to the string "golems" so the test verifies correct
decoding/fallback behavior of normalize_project_name.
- Around line 26-30: The test comment is misleading: update the docstring in
test_claude_code_encoded_path to state that
normalize_project_name("-Users-etanheyman-Gits-golems") will return "golems"
even if the filesystem path /Users/etanheyman/Gits/golems does not exist because
normalize_project_name checks os.path.isdir(...) and falls back to the last
segment (remaining[0]); change the comment to reflect this behavior (mention
normalize_project_name and the fallback) instead of claiming the test requires
the actual filesystem to have ~/Gits/golems.
- Around line 39-53: The test test_compound_name_with_dashes has a no-op
entry.replace('-', '-') and silently skips when ~/Gits is missing; change it to
explicitly skip with pytest.skip if gits_dir doesn't exist, and fix the encode
step to transform a name into the expected encoded form (e.g., replace path
separators with '-' as intended) before calling normalize_project_name; finally
tighten the assertion to check that normalize_project_name(encoded) returns the
expected decoded project path or name (reference normalize_project_name and
test_compound_name_with_dashes to locate the change).
In `@tests/test_paths.py`:
- Around line 15-46: The three tests in tests/test_paths.py
(test_env_var_override, test_legacy_path_if_exists,
test_canonical_path_fresh_install) duplicate behavior already covered by
TestDBPathResolution tests in test_phase3_qa.py; remove these redundant test
functions from tests/test_paths.py or consolidate them into the existing
TestDBPathResolution class to avoid duplication, keeping a single authoritative
implementation for get_db_path() behavior (env var override, legacy path
precedence, and canonical path creation) and ensuring any fixtures/patches
(patch.dict, patch("brainlayer.paths._LEGACY_DB_PATH"),
patch("brainlayer.paths._CANONICAL_DB_PATH")) remain in the one retained test
location so functionality and assertions (including canonical.parent.exists())
are preserved.
- Around line 48-52: The test_real_db_exists currently unconditionally requires
a production DB file at DEFAULT_DB_PATH and will fail in CI; update this test to
skip when the DB file is not present or not large enough by adding a
pytest.mark.skipif guard that checks DEFAULT_DB_PATH.is_file() and
DEFAULT_DB_PATH.stat().st_size > 1_000_000 (or use get_db_path() logic) so the
assertion only runs in environments with a real DB, or move the test into an
integration-only suite (e.g., tests/integration/) excluded from default CI runs.
In `@tests/test_phase3_qa.py`:
- Around line 39-41: The os.environ.pop("BRAINLAYER_DB", None) call is redundant
because patch.dict(os.environ, {}, clear=True) already clears the environment;
replace the clear-all approach with a targeted removal so you don't wipe other
env vars: remove the os.environ.pop lines and instead patch only the specific
key (e.g., use patch.dict(os.environ, {"BRAINLAYER_DB": None}, clear=False) or,
preferably in pytest, use monkeypatch.delenv("BRAINLAYER_DB", raising=False)) to
ensure BRAINLAYER_DB is absent without clearing the whole environment; update
both occurrences that reference patch.dict and os.environ.pop accordingly.
- Around line 219-233: The test test_claude_code_simple_path currently creates
tmp_path and segments but never uses them, which obscures intent; either remove
the tmp_path fixture and the unused segments variable, or (recommended) use
tmp_path to build the input passed to normalize_project_name and make the
os.path.isdir mock depend on that constructed path: construct the input string
from tmp_path (e.g., include tmp_path.name or
tmp_path.joinpath("Gits/myproject") in the path), and set mock_isdir.side_effect
to return True for that exact path (or for paths ending with "myproject") so
normalize_project_name is actually validated against the intended temporary
filesystem entry.
- Around line 376-401: Tests directly use store.conn.cursor() which couples
tests to VectorStore internals; add a small public helper on VectorStore named
get_rows(sql, params=()) that opens a cursor on self.conn, executes the query
with params and returns cursor.fetchall(), then update tests (e.g., the checks
in test_created_at_null_when_missing and the earlier dated-chunk test) to call
store.get_rows("SELECT created_at FROM chunks WHERE id = ?", ("undated-chunk",))
or the equivalent SQL/params instead of accessing .conn directly; this preserves
encapsulation and avoids exposing internal attribute usage.
In `@tests/test_vector_store.py`:
- Around line 112-114: Replace the conditional no-op with a real assertion that
the search returned at least one document, then perform the metadata assertion
unconditionally; specifically, assert that len(results["documents"]) > 0 (or use
pytest.assume to allow soft failures) before accessing
results["metadatas"][0][0], then assert "created_at" in that meta. Apply the
same change for the other occurrence referencing results["documents"] /
results["metadatas"] (the block around the second assertion at lines 128–130).
- Around line 144-146: The test test_stats_total_chunks hardcodes a
developer-specific threshold (assert stats["total_chunks"] > 200_000); change it
to a portable check by replacing the magic number with a configurable constant
(e.g., TEST_MIN_TOTAL_CHUNKS) defined in conftest.py or pytest.ini, or at
minimum assert stats["total_chunks"] > 0; update the assertion in
test_stats_total_chunks to use that constant (or > 0) and keep the call to
store.get_stats() unchanged.
- Around line 46-94: The three smoke tests (test_search_with_date_from,
test_search_with_date_to, test_search_with_date_range) only assert
isinstance(..., list) so they don't verify date filtering; update these tests to
assert that returned document IDs or counts respect the provided
date_from/date_to range (e.g., check all returned chunk timestamps are within
the range or that known out-of-range IDs are absent) using the
results["documents"] and any timestamp/id fields the store returns, or remove
the redundant smoke-class and rely on the controlled checks in
test_phase3_qa.py; also avoid repeatedly loading the sentence-transformers model
by hoisting get_embedding_model() into a module or session-scoped pytest fixture
(referencing get_embedding_model and the three test_* functions) and use that
fixture in the tests so the model is instantiated once.
- Around line 9-14: The fixture instantiates VectorStore(DEFAULT_DB_PATH)
unguarded which fails in CI when the production DB file is missing; wrap the
store fixture creation with a pytest.skipif guard (or add a skip_no_prod_db
marker) that checks for the DB file existence or a PROD_DB_AVAILABLE env var
before instantiating VectorStore, yielding and then calling store.close() as
before (VectorStore.close() and store.conn remain unchanged); also replace the
hardcoded assertion on total_chunks (> 200_000) with a non-production-dependent
check (e.g., assert total_chunks >= 1 or remove the assertion) so tests don't
rely on a specific production chunk count.
| class TestNormalizeProjectName: | ||
| """Test Claude Code path encoding → clean project name.""" | ||
|
|
||
| def test_none_returns_none(self): | ||
| assert normalize_project_name(None) is None | ||
|
|
||
| def test_empty_returns_none(self): | ||
| assert normalize_project_name("") is None | ||
| assert normalize_project_name(" ") is None | ||
| assert normalize_project_name("-") is None | ||
|
|
||
| def test_simple_name_passthrough(self): | ||
| """Already-clean names pass through unchanged.""" | ||
| assert normalize_project_name("golems") == "golems" | ||
|
|
||
| def test_claude_code_encoded_path(self): | ||
| """Standard Claude Code path encoding decodes correctly.""" | ||
| # This test requires the actual filesystem to have ~/Gits/golems | ||
| result = normalize_project_name("-Users-etanheyman-Gits-golems") | ||
| assert result == "golems" | ||
|
|
||
| def test_desktop_gits_path(self): | ||
| """Old Desktop/Gits paths decode correctly.""" | ||
| # Old path — directory may not exist, but first segment is returned | ||
| result = normalize_project_name("-Users-etanheyman-Desktop-Gits-golems") | ||
| # Either finds the dir or falls back to first segment | ||
| assert result is not None | ||
|
|
||
| def test_compound_name_with_dashes(self): | ||
| """Compound project names with dashes resolve via filesystem check.""" | ||
| # Only works if the directory actually exists | ||
| home = os.path.expanduser("~") | ||
| gits_dir = os.path.join(home, "Gits") | ||
| if os.path.isdir(gits_dir): | ||
| for entry in os.listdir(gits_dir): | ||
| if "-" in entry and os.path.isdir(os.path.join(gits_dir, entry)): | ||
| # Test that this compound name resolves correctly | ||
| encoded = f"-Users-{os.path.basename(home)}-Gits-{entry.replace('-', '-')}" | ||
| # The encoding is just dashes, same as the name | ||
| # Just verify it doesn't crash | ||
| result = normalize_project_name(encoded) | ||
| assert result is not None | ||
| break | ||
|
|
||
| def test_worktree_suffix_stripped(self): | ||
| """Worktree suffixes are stripped.""" | ||
| assert normalize_project_name("golems-nightshift-1770775282043") == "golems" | ||
| assert normalize_project_name("golems-haiku-1770775282043") == "golems" | ||
| assert normalize_project_name("golems-worktree-1770775282043") == "golems" | ||
|
|
||
| def test_no_gits_segment_returns_none(self): | ||
| """Paths without 'Gits' segment return None.""" | ||
| result = normalize_project_name("-Users-etanheyman-Documents-stuff") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
TestNormalizeProjectName substantially duplicates TestProjectNameNormalization in tests/test_phase3_qa.py.
The following test cases are either identical or near-identical across the two files:
test_normalize_project.py |
test_phase3_qa.py |
|---|---|
test_none_returns_none |
test_none_input |
test_empty_returns_none |
test_empty_string, test_dash_only, test_whitespace |
test_simple_name_passthrough |
test_clean_name_passthrough |
test_worktree_suffix_stripped |
test_worktree_suffix_stripped |
test_no_gits_segment_returns_none |
test_no_gits_segment |
Consider consolidating all normalization tests into a single file to reduce maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_normalize_project.py` around lines 11 - 63, The
TestNormalizeProjectName test class duplicates many cases already covered by
TestProjectNameNormalization; consolidate by removing the redundant tests (or
the entire TestNormalizeProjectName class) and keep a single canonical test
suite (e.g., TestProjectNameNormalization) that includes the unique cases;
locate the duplicated test methods by name (test_none_returns_none /
test_none_input, test_empty_returns_none /
test_empty_string|test_dash_only|test_whitespace, test_simple_name_passthrough /
test_clean_name_passthrough, test_worktree_suffix_stripped,
test_no_gits_segment_returns_none / test_no_gits_segment) and either move any
unique assertions into the canonical class or delete them, then run the test
suite to ensure no missing imports or references to normalize_project_name
remain.
| def test_claude_code_encoded_path(self): | ||
| """Standard Claude Code path encoding decodes correctly.""" | ||
| # This test requires the actual filesystem to have ~/Gits/golems | ||
| result = normalize_project_name("-Users-etanheyman-Gits-golems") | ||
| assert result == "golems" |
There was a problem hiding this comment.
Misleading comment — this test passes on every machine regardless of filesystem.
The comment "This test requires the actual filesystem to have ~/Gits/golems" is incorrect. For the input "-Users-etanheyman-Gits-golems", the implementation tries os.path.isdir("/Users/etanheyman/Gits/golems") and falls back to remaining[0] — which is "golems" — when the directory is absent. The assertion result == "golems" therefore always passes, not because the directory exists, but due to the fallback. The comment should be corrected to avoid misleading future maintainers.
🛠️ Proposed fix
def test_claude_code_encoded_path(self):
"""Standard Claude Code path encoding decodes correctly."""
- # This test requires the actual filesystem to have ~/Gits/golems
+ # Falls back to the first remaining segment ("golems") when directory isn't found.
result = normalize_project_name("-Users-etanheyman-Gits-golems")
assert result == "golems"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_normalize_project.py` around lines 26 - 30, The test comment is
misleading: update the docstring in test_claude_code_encoded_path to state that
normalize_project_name("-Users-etanheyman-Gits-golems") will return "golems"
even if the filesystem path /Users/etanheyman/Gits/golems does not exist because
normalize_project_name checks os.path.isdir(...) and falls back to the last
segment (remaining[0]); change the comment to reflect this behavior (mention
normalize_project_name and the fallback) instead of claiming the test requires
the actual filesystem to have ~/Gits/golems.
| def test_desktop_gits_path(self): | ||
| """Old Desktop/Gits paths decode correctly.""" | ||
| # Old path — directory may not exist, but first segment is returned | ||
| result = normalize_project_name("-Users-etanheyman-Desktop-Gits-golems") | ||
| # Either finds the dir or falls back to first segment | ||
| assert result is not None |
There was a problem hiding this comment.
Weak assertion in test_desktop_gits_path — result is not None doesn't verify decoding.
For "-Users-etanheyman-Desktop-Gits-golems" the fallback in normalize_project_name always returns "golems" (same analysis as test_claude_code_encoded_path), so the assertion can and should be strengthened:
🛠️ Proposed fix
- # Either finds the dir or falls back to first segment
- assert result is not None
+ assert result == "golems" # Fallback always returns first remaining segment📝 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.
| def test_desktop_gits_path(self): | |
| """Old Desktop/Gits paths decode correctly.""" | |
| # Old path — directory may not exist, but first segment is returned | |
| result = normalize_project_name("-Users-etanheyman-Desktop-Gits-golems") | |
| # Either finds the dir or falls back to first segment | |
| assert result is not None | |
| def test_desktop_gits_path(self): | |
| """Old Desktop/Gits paths decode correctly.""" | |
| # Old path — directory may not exist, but first segment is returned | |
| result = normalize_project_name("-Users-etanheyman-Desktop-Gits-golems") | |
| assert result == "golems" # Fallback always returns first remaining segment |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_normalize_project.py` around lines 32 - 37, The
test.test_desktop_gits_path currently asserts only that
normalize_project_name("-Users-etanheyman-Desktop-Gits-golems") is not None;
strengthen it to verify the actual decoded/fallback value returned by
normalize_project_name is "golems" (same expectation as
test_claude_code_encoded_path). Update the assertion in test_desktop_gits_path
to compare the result to the string "golems" so the test verifies correct
decoding/fallback behavior of normalize_project_name.
| def test_compound_name_with_dashes(self): | ||
| """Compound project names with dashes resolve via filesystem check.""" | ||
| # Only works if the directory actually exists | ||
| home = os.path.expanduser("~") | ||
| gits_dir = os.path.join(home, "Gits") | ||
| if os.path.isdir(gits_dir): | ||
| for entry in os.listdir(gits_dir): | ||
| if "-" in entry and os.path.isdir(os.path.join(gits_dir, entry)): | ||
| # Test that this compound name resolves correctly | ||
| encoded = f"-Users-{os.path.basename(home)}-Gits-{entry.replace('-', '-')}" | ||
| # The encoding is just dashes, same as the name | ||
| # Just verify it doesn't crash | ||
| result = normalize_project_name(encoded) | ||
| assert result is not None | ||
| break |
There was a problem hiding this comment.
test_compound_name_with_dashes has a no-op string operation and is vacuously true in CI.
Two issues:
-
No-op:
entry.replace('-', '-')on line 48 replaces'-'with'-'and does nothing. This is almost certainly a typo (perhaps intended asentry.replace('/', '-')to simulate a Claude Code path encoding). -
Vacuous pass: The entire test body is wrapped in
if os.path.isdir(gits_dir):(line 44). On any machine where~/Gitsdoesn't exist (all CI runners), the test passes trivially without exercising any logic. Even when~/Gitsexists, the inner loop exits onbreakat the first matching entry with onlyassert result is not None— not verifying the decoded value.
Use pytest.skip to make the environment dependency explicit, and tighten the assertion:
🛠️ Proposed fix
def test_compound_name_with_dashes(self):
"""Compound project names with dashes resolve via filesystem check."""
home = os.path.expanduser("~")
gits_dir = os.path.join(home, "Gits")
if not os.path.isdir(gits_dir):
+ pytest.skip("~/Gits not present on this machine")
for entry in os.listdir(gits_dir):
if "-" in entry and os.path.isdir(os.path.join(gits_dir, entry)):
- encoded = f"-Users-{os.path.basename(home)}-Gits-{entry.replace('-', '-')}"
+ # Claude Code encodes path separators as dashes; the name itself uses dashes
+ encoded = f"-Users-{os.path.basename(home)}-Gits-{entry}"
result = normalize_project_name(encoded)
- assert result is not None
+ assert result == entry, f"Expected {entry!r}, got {result!r}"
break
+ else:
+ pytest.skip("No compound-name directories found in ~/Gits")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_normalize_project.py` around lines 39 - 53, The test
test_compound_name_with_dashes has a no-op entry.replace('-', '-') and silently
skips when ~/Gits is missing; change it to explicitly skip with pytest.skip if
gits_dir doesn't exist, and fix the encode step to transform a name into the
expected encoded form (e.g., replace path separators with '-' as intended)
before calling normalize_project_name; finally tighten the assertion to check
that normalize_project_name(encoded) returns the expected decoded project path
or name (reference normalize_project_name and test_compound_name_with_dashes to
locate the change).
| def test_env_var_override(self, tmp_path): | ||
| """BRAINLAYER_DB env var takes highest priority.""" | ||
| db_path = tmp_path / "custom.db" | ||
| with patch.dict(os.environ, {"BRAINLAYER_DB": str(db_path)}): | ||
| assert get_db_path() == db_path | ||
|
|
||
| def test_legacy_path_if_exists(self, tmp_path): | ||
| """Legacy zikaron path used when it exists.""" | ||
| legacy = tmp_path / "zikaron.db" | ||
| legacy.touch() | ||
| with ( | ||
| patch.dict(os.environ, {}, clear=True), | ||
| patch("brainlayer.paths._LEGACY_DB_PATH", legacy), | ||
| patch("brainlayer.paths._CANONICAL_DB_PATH", tmp_path / "brainlayer.db"), | ||
| ): | ||
| # Remove env var if set | ||
| os.environ.pop("BRAINLAYER_DB", None) | ||
| assert get_db_path() == legacy | ||
|
|
||
| def test_canonical_path_fresh_install(self, tmp_path): | ||
| """Canonical path used when no legacy DB exists.""" | ||
| canonical = tmp_path / "brainlayer" / "brainlayer.db" | ||
| legacy = tmp_path / "nonexistent" / "zikaron.db" | ||
| with ( | ||
| patch.dict(os.environ, {}, clear=True), | ||
| patch("brainlayer.paths._LEGACY_DB_PATH", legacy), | ||
| patch("brainlayer.paths._CANONICAL_DB_PATH", canonical), | ||
| ): | ||
| os.environ.pop("BRAINLAYER_DB", None) | ||
| result = get_db_path() | ||
| assert result == canonical | ||
| assert canonical.parent.exists() # Parent dir created |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
TestGetDbPath largely duplicates TestDBPathResolution in test_phase3_qa.py.
test_env_var_override, test_legacy_path_if_exists, and test_canonical_path_fresh_install are functionally equivalent to TestDBPathResolution.test_env_var_override, test_legacy_path_preferred_over_canonical, and test_canonical_path_when_legacy_missing respectively. Consider consolidating into a single location to reduce maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_paths.py` around lines 15 - 46, The three tests in
tests/test_paths.py (test_env_var_override, test_legacy_path_if_exists,
test_canonical_path_fresh_install) duplicate behavior already covered by
TestDBPathResolution tests in test_phase3_qa.py; remove these redundant test
functions from tests/test_paths.py or consolidate them into the existing
TestDBPathResolution class to avoid duplication, keeping a single authoritative
implementation for get_db_path() behavior (env var override, legacy path
precedence, and canonical path creation) and ensuring any fixtures/patches
(patch.dict, patch("brainlayer.paths._LEGACY_DB_PATH"),
patch("brainlayer.paths._CANONICAL_DB_PATH")) remain in the one retained test
location so functionality and assertions (including canonical.parent.exists())
are preserved.
| store.upsert_chunks(chunks, [[0.1] * 1024]) | ||
|
|
||
| cursor = store.conn.cursor() | ||
| row = list(cursor.execute("SELECT created_at FROM chunks WHERE id = 'dated-chunk'")) | ||
| assert row[0][0] == "2026-02-19T10:00:00+00:00" | ||
|
|
||
| def test_created_at_null_when_missing(self, tmp_path): | ||
| """Chunks without created_at should have NULL in the DB.""" | ||
| store = VectorStore(tmp_path / "test.db") | ||
| chunks = [ | ||
| { | ||
| "id": "undated-chunk", | ||
| "content": "Test content without a date", | ||
| "metadata": {}, | ||
| "source_file": "/test.jsonl", | ||
| "project": "test", | ||
| "content_type": "user_message", | ||
| "char_count": 30, | ||
| "source": "claude_code", | ||
| } | ||
| ] | ||
| store.upsert_chunks(chunks, [[0.1] * 1024]) | ||
|
|
||
| cursor = store.conn.cursor() | ||
| row = list(cursor.execute("SELECT created_at FROM chunks WHERE id = 'undated-chunk'")) | ||
| assert row[0][0] is None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the VectorStore class definition
fd -e py -type f | xargs grep -l "class VectorStore"Repository: EtanHey/brainlayer
Length of output: 234
🏁 Script executed:
# Look for the test file and examine lines 376-401
fd test_phase3_qa.py -type fRepository: EtanHey/brainlayer
Length of output: 232
🏁 Script executed:
# Get the context of VectorStore class
ast-grep --pattern 'class VectorStore {
$$$
}'Repository: EtanHey/brainlayer
Length of output: 44
🏁 Script executed:
# Find VectorStore class definition
fd VectorStore -e pyRepository: EtanHey/brainlayer
Length of output: 44
🏁 Script executed:
# Find test_phase3_qa.py file
fd test_phase3_qa -e pyRepository: EtanHey/brainlayer
Length of output: 85
🏁 Script executed:
# Search for VectorStore class in all Python files
rg "class VectorStore" -A 20Repository: EtanHey/brainlayer
Length of output: 1507
🏁 Script executed:
# Read the entire VectorStore implementation
cat -n src/brainlayer/vector_store.pyRepository: EtanHey/brainlayer
Length of output: 50374
🏁 Script executed:
# Read the relevant test lines 376-401
sed -n '376,401p' tests/test_phase3_qa.pyRepository: EtanHey/brainlayer
Length of output: 1033
Consider exposing a test helper method instead of direct .conn access.
Both tests reach into store.conn.cursor() to query the DB directly. While .conn is currently a public attribute, tests that depend on internal implementation details risk silent breakage if the API evolves (e.g., wrapping the connection, switching to a context-manager pattern, or renaming the attribute). Expose a get_rows(sql, params) method on VectorStore for test queries, or document .conn as a stable public contract and add an __all__ export list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_phase3_qa.py` around lines 376 - 401, Tests directly use
store.conn.cursor() which couples tests to VectorStore internals; add a small
public helper on VectorStore named get_rows(sql, params=()) that opens a cursor
on self.conn, executes the query with params and returns cursor.fetchall(), then
update tests (e.g., the checks in test_created_at_null_when_missing and the
earlier dated-chunk test) to call store.get_rows("SELECT created_at FROM chunks
WHERE id = ?", ("undated-chunk",)) or the equivalent SQL/params instead of
accessing .conn directly; this preserves encapsulation and avoids exposing
internal attribute usage.
| @pytest.fixture(scope="module") | ||
| def store(): | ||
| """Read-only connection to the production DB for integration tests.""" | ||
| s = VectorStore(DEFAULT_DB_PATH) | ||
| yield s | ||
| s.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check VectorStore class for close() method and .conn attribute
fd -e py -g '*vector_store*' src --exec grep -n "def close\|self\.conn\b"Repository: EtanHey/brainlayer
Length of output: 1562
🏁 Script executed:
# Find and examine VectorStore class definition
fd -e py src -type f -name '*vector_store*'Repository: EtanHey/brainlayer
Length of output: 232
🏁 Script executed:
# Check what DEFAULT_DB_PATH is and if it's Path-like
rg "DEFAULT_DB_PATH\s*=" --no-heading -A 2 srcRepository: EtanHey/brainlayer
Length of output: 379
🏁 Script executed:
# Verify VectorStore class structure and __init__ signature
ast-grep --pattern 'class VectorStore {
$$$
def __init__($$$) {
$$$
}
$$$
}'Repository: EtanHey/brainlayer
Length of output: 44
🏁 Script executed:
# Find and read get_db_path() function
rg "def get_db_path" -A 10 srcRepository: EtanHey/brainlayer
Length of output: 670
🏁 Script executed:
# Check paths.py to understand DEFAULT_DB_PATH
cat src/brainlayer/paths.pyRepository: EtanHey/brainlayer
Length of output: 1410
🏁 Script executed:
# Verify how test_vector_store.py imports DEFAULT_DB_PATH
head -20 tests/test_vector_store.pyRepository: EtanHey/brainlayer
Length of output: 556
🏁 Script executed:
# Read the full test_vector_store.py to verify all observations
cat -n tests/test_vector_store.pyRepository: EtanHey/brainlayer
Length of output: 6193
Production DB dependency causes fixture to fail in CI — VectorStore.close() and store.conn are valid, but add skipif guard and fix hardcoded count.
The fixture at lines 9–14 calls VectorStore(DEFAULT_DB_PATH) unconditionally. If the production DB file doesn't exist (any CI environment, container, or fresh developer clone), the instantiation fails at test runtime. While VectorStore.close() and VectorStore.conn both exist and are correctly implemented, the fixture needs a skip guard. Additionally, line 146 hardcodes total_chunks > 200_000 — a production-specific fact that will fail on any machine with fewer chunks.
🛠️ Proposed fix
+import pytest
+
from brainlayer.paths import DEFAULT_DB_PATH
from brainlayer.vector_store import VectorStore
+skip_no_prod_db = pytest.mark.skipif(
+ not DEFAULT_DB_PATH.exists(),
+ reason="Production DB not present on this machine",
+)
+
+
`@pytest.fixture`(scope="module")
+@skip_no_prod_db
def store():
"""Read-only connection to the production DB for integration tests."""
s = VectorStore(DEFAULT_DB_PATH)
yield s
s.close()Then annotate test classes with @skip_no_prod_db or use pytestmark = skip_no_prod_db. For line 146, either make the count dynamic (>= 1) or remove the assertion if total chunk count is not a meaningful test condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_vector_store.py` around lines 9 - 14, The fixture instantiates
VectorStore(DEFAULT_DB_PATH) unguarded which fails in CI when the production DB
file is missing; wrap the store fixture creation with a pytest.skipif guard (or
add a skip_no_prod_db marker) that checks for the DB file existence or a
PROD_DB_AVAILABLE env var before instantiating VectorStore, yielding and then
calling store.close() as before (VectorStore.close() and store.conn remain
unchanged); also replace the hardcoded assertion on total_chunks (> 200_000)
with a non-production-dependent check (e.g., assert total_chunks >= 1 or remove
the assertion) so tests don't rely on a specific production chunk count.
| def test_search_with_date_from(self, store): | ||
| """Search with date_from filter returns only recent results.""" | ||
| from brainlayer.embeddings import get_embedding_model | ||
|
|
||
| model = get_embedding_model() | ||
| query_emb = model.embed_query("test query") | ||
|
|
||
| results = store.hybrid_search( | ||
| query_embedding=query_emb, | ||
| query_text="test query", | ||
| n_results=5, | ||
| date_from="2026-02-15", | ||
| ) | ||
| docs = results["documents"][0] | ||
| # Should return results (we have data from Feb 2026) | ||
| # The key test is that it doesn't crash | ||
| assert isinstance(docs, list) | ||
|
|
||
| def test_search_with_date_to(self, store): | ||
| """Search with date_to filter works.""" | ||
| from brainlayer.embeddings import get_embedding_model | ||
|
|
||
| model = get_embedding_model() | ||
| query_emb = model.embed_query("test query") | ||
|
|
||
| results = store.hybrid_search( | ||
| query_embedding=query_emb, | ||
| query_text="test query", | ||
| n_results=5, | ||
| date_to="2026-01-01", | ||
| ) | ||
| # Should not crash, may return empty if no old data | ||
| assert isinstance(results["documents"][0], list) | ||
|
|
||
| def test_search_with_date_range(self, store): | ||
| """Search with both date_from and date_to works.""" | ||
| from brainlayer.embeddings import get_embedding_model | ||
|
|
||
| model = get_embedding_model() | ||
| query_emb = model.embed_query("authentication") | ||
|
|
||
| results = store.hybrid_search( | ||
| query_embedding=query_emb, | ||
| query_text="authentication", | ||
| n_results=5, | ||
| date_from="2026-02-01", | ||
| date_to="2026-02-28", | ||
| ) | ||
| assert isinstance(results["documents"][0], list) |
There was a problem hiding this comment.
TestDateFiltering smoke tests assert only isinstance(docs, list) — date filtering is not verified.
All three integration tests in this class only check that hybrid_search doesn't raise, not that the date filter actually excludes out-of-range chunks. The meaningful date-filtering assertions live in test_phase3_qa.py against a controlled fixture. Consider either adding result-count or ID checks here, or removing the class and relying solely on the controlled tests — the smoke tests as written give false confidence.
Additionally, get_embedding_model() is called once per test function (lines 50, 67, 84). If the model is not internally cached, this loads the sentence-transformers model three times, making the suite unnecessarily slow.
🛠️ Proposed refactor — hoist model to fixture
+@pytest.fixture(scope="module")
+def query_embedding(store):
+ from brainlayer.embeddings import get_embedding_model
+ model = get_embedding_model()
+ return model.embed_query("test query")
+
+
class TestDateFiltering:
- def test_search_with_date_from(self, store):
- from brainlayer.embeddings import get_embedding_model
- model = get_embedding_model()
- query_emb = model.embed_query("test query")
+ def test_search_with_date_from(self, store, query_embedding):
results = store.hybrid_search(
- query_embedding=query_emb,
+ query_embedding=query_embedding,
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_vector_store.py` around lines 46 - 94, The three smoke tests
(test_search_with_date_from, test_search_with_date_to,
test_search_with_date_range) only assert isinstance(..., list) so they don't
verify date filtering; update these tests to assert that returned document IDs
or counts respect the provided date_from/date_to range (e.g., check all returned
chunk timestamps are within the range or that known out-of-range IDs are absent)
using the results["documents"] and any timestamp/id fields the store returns, or
remove the redundant smoke-class and rely on the controlled checks in
test_phase3_qa.py; also avoid repeatedly loading the sentence-transformers model
by hoisting get_embedding_model() into a module or session-scoped pytest fixture
(referencing get_embedding_model and the three test_* functions) and use that
fixture in the tests so the model is instantiated once.
| if results["documents"][0]: | ||
| meta = results["metadatas"][0][0] | ||
| assert "created_at" in meta, "Search results should include created_at" |
There was a problem hiding this comment.
Conditional assertions silently skip when results are empty.
if results["documents"][0]:
meta = results["metadatas"][0][0]
assert "created_at" in meta, ...If the query returns zero results (entirely possible for date_to="2026-01-01" or if the embedding aligns poorly), the assertion never fires and the test vacuously passes. Use pytest.assume or assert unconditionally after ensuring at least one result is returned, to avoid silent no-ops.
Also applies to: 128-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_vector_store.py` around lines 112 - 114, Replace the conditional
no-op with a real assertion that the search returned at least one document, then
perform the metadata assertion unconditionally; specifically, assert that
len(results["documents"]) > 0 (or use pytest.assume to allow soft failures)
before accessing results["metadatas"][0][0], then assert "created_at" in that
meta. Apply the same change for the other occurrence referencing
results["documents"] / results["metadatas"] (the block around the second
assertion at lines 128–130).
| def test_stats_total_chunks(self, store): | ||
| stats = store.get_stats() | ||
| assert stats["total_chunks"] > 200_000 # We have 268K+ |
There was a problem hiding this comment.
test_stats_total_chunks hardcodes a developer-machine-specific row count.
assert stats["total_chunks"] > 200_000 encodes a fact about the current state of one developer's production database and will fail on any other machine. A portable assertion would simply check stats["total_chunks"] > 0 (or > some_reasonable_lower_bound set via a pytest.ini/conftest constant).
🛠️ Proposed fix
- def test_stats_total_chunks(self):
- stats = store.get_stats()
- assert stats["total_chunks"] > 200_000 # We have 268K+
+ def test_stats_total_chunks(self, store):
+ stats = store.get_stats()
+ assert stats["total_chunks"] > 0, "Expected at least some chunks in the DB"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_vector_store.py` around lines 144 - 146, The test
test_stats_total_chunks hardcodes a developer-specific threshold (assert
stats["total_chunks"] > 200_000); change it to a portable check by replacing the
magic number with a configurable constant (e.g., TEST_MIN_TOTAL_CHUNKS) defined
in conftest.py or pytest.ini, or at minimum assert stats["total_chunks"] > 0;
update the assertion in test_stats_total_chunks to use that constant (or > 0)
and keep the call to store.get_stats() unchanged.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| from brainlayer.paths import DEFAULT_DB_PATH | ||
|
|
||
| assert DEFAULT_DB_PATH.exists(), f"DB not found at {DEFAULT_DB_PATH}" | ||
| assert DEFAULT_DB_PATH.stat().st_size > 1_000_000, "DB too small — might be empty" |
There was a problem hiding this comment.
Test asserts production DB exists, fails CI
Medium Severity
test_real_db_exists asserts DEFAULT_DB_PATH.exists() and st_size > 1_000_000, which only passes on the developer's machine where the production database is present. This test will fail in CI or any fresh environment, since no production database exists there.
* test: Phase 4 QA — comprehensive tests for Phase 3 core fixes 28 new tests covering: - DB path resolution (env var, legacy, canonical) - Date filtering (date_from, date_to, ranges) in search + hybrid_search - Project name normalization (Claude Code paths, worktrees, compound names) - Search metadata (created_at, source in results) - Chunk boundary improvements (sentence-aware splitting) - created_at storage in upsert_chunks Full suite: 160 passed, 2 skipped, 0 failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add QA tests for Phase 3 — paths, normalization, chunker, search - test_paths.py: DB path resolution order (env, legacy, canonical) - test_normalize_project.py: Claude Code path encoding, worktree stripping - test_chunker.py: sentence-aware splitting, content-type filtering - test_vector_store.py: schema verification, date filtering, search metadata Unit tests: 19/19 pass (paths, normalization, chunker) Integration tests: 3/3 schema tests pass, search tests need embedding model Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Mostly test-only changes, but several new tests depend on the presence/size of a real production DB and filesystem paths, which may introduce CI/environment brittleness.
Overview
Adds new QA-focused pytest coverage for Phase 3 behaviors:
get_db_pathprecedence (env override, legacy vs canonical),normalize_project_namedecoding (Claude Code path encodings + worktree suffix stripping), andVectorStoresearch/hybrid-searchdate_from/date_tofiltering plus requiredcreated_at/sourcemetadata.Refactors
test_vector_store.pyfrom pure unit tests into integration-style checks againstDEFAULT_DB_PATH, asserting schema/backfill coverage and basicget_statsexpectations.Written by Cursor Bugbot for commit ea52424. This will update automatically on new commits. Configure here.
Summary by CodeRabbit