Skip to content

Semantic codebase search#113

Merged
m-aebrer merged 11 commits intomasterfrom
feature/issue-108-semantic-codebase-search
Apr 7, 2026
Merged

Semantic codebase search#113
m-aebrer merged 11 commits intomasterfrom
feature/issue-108-semantic-codebase-search

Conversation

@m-aebrer
Copy link
Copy Markdown
Collaborator

@m-aebrer m-aebrer commented Apr 7, 2026

Closes #108

Adds a built-in search tool that uses embeddings + FTS5 to support natural language queries over the codebase. Uses POEM-based multi-metric ranking (FTS5 BM25, vector cosine, path match, symbol match, import graph, git recency) with AST-aware code chunking via tree-sitter.

Key design: zero native addons — uses node:sqlite (built-in Node 22), web-tree-sitter (WASM), and @huggingface/transformers (WASM).

Implementation plan posted as a comment below.

@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Implementation Plan

Analysis

Issue 108 adds a semantic codebase search tool. Eight design decisions are already finalized: local-only embeddings (ONNX), AST-aware chunking (tree-sitter), project-scoped index (.dreb/index/) + global memory index (~/.dreb/index/), memory files indexed alongside code, lazy incremental indexing, built-in tool, nomic-embed-text-v1.5, and POEM-based multi-metric ranking.

Key discovery — zero native addons possible:

The original assessment flagged native addon friction as a top risk (better-sqlite3 + tree-sitter + onnxruntime-node). But:

Component Native addon approach Zero-addon approach
SQLite better-sqlite3 node:sqlite (built-in Node 22) — FTS5 + BM25 confirmed working on Node 22.20
Tree-sitter tree-sitter (native) web-tree-sitter (WASM)
Embeddings onnxruntime-node @huggingface/transformers (WASM default)

This keeps dreb's zero native addon status intact. The search tool is feature-gated on node:sqlite availability — users on Node <22 just don't get it (no crash). Node 20 EOL is April 2026, so this is a natural transition.

Deliverables

  1. search tool — built-in tool (same tier as grep/find/ls) accepting natural language queries, returning ranked code/doc results
  2. Indexing subsystem — SQLite-backed index with FTS5 + vector storage, AST-aware code chunking, incremental mtime-based updates
  3. POEM ranking engine — 6-metric Pareto-optimal ranking
  4. Model management — auto-download and cache nomic-embed-text-v1.5
  5. TUI integration — renderCall/renderResult for search results
  6. Tests — unit + integration tests for all components

Acceptance Criteria

  • search tool appears in system prompt "Available tools" section
  • First query triggers index build with progress reporting; subsequent queries use cached index
  • Incremental re-indexing: only changed files (by mtime) are re-processed
  • Short identifier queries bias toward FTS5 exact matches via column duplication
  • Long natural language queries bias toward vector similarity via column duplication
  • Memory files (project .dreb/memory/ + global ~/.dreb/memory/) searchable alongside code
  • Index stored at .dreb/index/ (project) and ~/.dreb/index/ (global memory)
  • nomic-embed-text-v1.5 auto-downloads on first use, cached at ~/.dreb/agent/models/
  • Tree-sitter produces function/class/method/export chunks for: TS/JS, Python, Go, Rust, Java, C/C++, GDScript
  • Non-code files (markdown, YAML, JSON, TOML, plain text) chunked by format-appropriate boundaries
  • Unrecognized code languages raise an error (not silent fallback)
  • All 6 POEM metrics: FTS5 BM25, vector cosine, path match, symbol match, import graph proximity, git recency
  • POEM: top-1000 pruning per metric → Pareto front → mean-distance tiebreak
  • Embedding schema keyed by model name (supports future multi-model addition)
  • Works offline after model download
  • Gracefully unavailable on Node <22 (tool not registered, no crash)
  • No regression in existing tools or tests

Files to Create

Search subsystem (packages/coding-agent/src/core/search/):

File Purpose
types.ts Shared types: Chunk, SearchResult, Metric, IndexConfig, ChunkType, Language
db.ts SQLite abstraction over node:sqlite; schema creation (files, chunks, chunks_fts, embeddings, imports, symbols); migrations
index-manager.ts Index lifecycle: create/open/close, mtime staleness check, incremental update orchestration, progress reporting via onUpdate callback
scanner.ts File discovery: walk project respecting .gitignore, detect language by extension, include memory files from .dreb/memory/ and ~/.dreb/memory/
chunker.ts Chunking coordinator: dispatch to tree-sitter or text chunker based on detected language
tree-sitter-chunker.ts AST-aware chunking via web-tree-sitter: extract functions, classes, methods, exports with per-language tree-sitter queries
text-chunker.ts Non-code chunking: markdown by heading, YAML/JSON/TOML by top-level key, plain text by paragraph
embedder.ts Embedding pipeline: model download to ~/.dreb/agent/models/, ONNX inference via @huggingface/transformers, batch processing, search_document:/search_query: prefixes
fts.ts FTS5 operations: index chunk content + name + path, BM25 query, score normalization to [0,1]
vector-store.ts Vector storage: pack/unpack float32 arrays as BLOBs, cosine similarity (dot product on normalized vectors)
metrics/bm25.ts FTS5 BM25 metric: keyword/identifier matching
metrics/cosine.ts Vector cosine similarity metric
metrics/path-match.ts Path/filename token overlap with query
metrics/symbol-match.ts Symbol name match: function/class/export names vs query terms
metrics/import-graph.ts Import graph proximity: files importing/imported by high-scoring files
metrics/git-recency.ts Git blame recency: recently modified chunks rank higher
poem.ts POEM ranking: top-K prune (union of top-1000 per metric) → Pareto front → mean-distance tiebreak; column duplication for query-type weighting
query-classifier.ts Query type detection: identifier (short, camelCase/snake_case) vs natural language (long, spaces) vs path-like (slashes, dots)
search.ts Main search API: check/build index → compute all 6 metrics → classify query → duplicate columns → POEM rank → assemble results

Tool definition:

File Purpose
core/tools/search.ts Tool factory: schema ({ query, path?, limit? }), execute, renderCall, renderResult, promptSnippet, promptGuidelines

Files to Modify

File Changes
core/tools/index.ts Add search to createAllToolDefinitions(), allTools, allToolDefinitions, exports, ToolName type
package.json (coding-agent) Add deps: web-tree-sitter, @huggingface/transformers, tree-sitter language grammar WASM packages

Testing Approach

Test files in packages/coding-agent/test/search/:

Test file Coverage
poem.test.ts POEM algorithm: prune, Pareto front, tiebreak, column duplication; edge cases (empty set, single candidate, all dominated, identical scores)
query-classifier.test.ts Identifiers (AuthMiddleware, create_user), natural language ("where is auth"), path-like (src/auth/), mixed cases
tree-sitter-chunker.test.ts AST chunking per language: correct boundaries, symbol extraction, nested structures, export detection
text-chunker.test.ts Markdown heading splits, YAML/JSON/TOML top-level key splits, plain text paragraph grouping
fts.test.ts FTS5 indexing, BM25 queries (exact, partial, multi-word), score normalization
vector-store.test.ts Float32 BLOB pack/unpack round-trip, cosine similarity correctness, batch operations
db.test.ts Schema creation, node:sqlite availability check, graceful degradation on Node <22
scanner.test.ts File discovery, .gitignore respect, language detection, memory file inclusion
index-manager.test.ts Index creation, mtime staleness detection, incremental update (add/modify/delete files)
metrics/*.test.ts Each metric: normalization to [0,1], correct ranking order, edge cases
search.test.ts Integration: end-to-end search over fixture project with pre-computed embeddings
search-tool.test.ts Tool definition: schema validation, execute output format, promptSnippet present, Node version gating

Test infrastructure:

  • Fixture directories with small, deterministic code files in multiple languages
  • Pre-computed embedding fixtures (avoid model download in CI)
  • In-memory SQLite databases for fast unit tests
  • Tree-sitter WASM loading helpers

Risks and Open Questions

  1. node:sqlite stability. Experimental in Node 22. Mitigation: abstracted behind db.ts; swappable to better-sqlite3 if bugs emerge. Feature-gated — tool simply doesn't register on Node <22.

  2. Tree-sitter GDScript grammar. Community-maintained WASM may not exist or be low quality. Verify during implementation; may drop from initial support if unavailable.

  3. Model download size. ~131MB on first use. Mitigation: progress reporting, permanent cache at ~/.dreb/agent/models/, works offline afterward.

  4. Import graph parsing. Language-specific import statement parsing (ES6 import, Python import/from, Go import, Rust use, Java import). Highest effort among the 6 metrics — each language needs its own import extractor.

  5. Git recency in shallow clones. git blame may fail. Mitigation: graceful degradation — exclude git recency column from POEM matrix when unavailable.

  6. WASM performance for initial indexing. web-tree-sitter and @huggingface/transformers WASM paths are slower than native. Large repos may take minutes for first index. Mitigation: batch processing, progress reporting, cached index.

  7. CI model download. Full embedding integration tests need the ONNX model. Mitigation: unit tests use pre-computed fixtures; a separate integration test suite handles real model loading.


Plan created by mach6

Add built-in `search` tool using embeddings + FTS5 for natural language
queries over the codebase. Zero native addons — uses node:sqlite (Node 22),
web-tree-sitter (WASM), and @huggingface/transformers (WASM/native).

Search subsystem (src/core/search/):
- AST-aware code chunking via tree-sitter (TS/JS/Python/Go/Rust/Java/C/C++)
- Format-aware text chunking (markdown/YAML/JSON/TOML/plaintext)
- nomic-embed-text-v1.5 embeddings with auto-download and caching
- SQLite-backed index with FTS5 + vector storage
- POEM/TFPR multi-metric ranking (6 metrics, column duplication weighting)
- Incremental mtime-based re-indexing
- File scanner respecting .gitignore with memory file inclusion

Metrics: FTS5 BM25, vector cosine, path match, symbol match, import graph
proximity, git recency. Query classifier biases ranking toward relevant
metrics (identifiers -> BM25+symbol, natural language -> cosine, paths -> path).

Feature-gated on node:sqlite — tool not registered on Node <22, no crash.

Tests: 109 new tests across 6 test files (poem, db, vector-store,
tree-sitter-chunker, search-tool, text-chunker).
@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Progress Update

Implemented: Full semantic search subsystem

29 files changed — 13,545 additions across 21 source files and 6 test files.

Search subsystem (src/core/search/)

  • types.ts — Shared types (Chunk, SearchResult, MetricScores, etc.)
  • db.ts — SQLite abstraction over node:sqlite with FTS5, embeddings, imports, symbols tables
  • scanner.ts — File discovery respecting .gitignore, detects file types, includes memory files
  • chunker.ts — Coordinator dispatching to tree-sitter or text chunker
  • tree-sitter-chunker.ts — AST-aware chunking for 9 languages (TS/TSX/JS/Python/Go/Rust/Java/C/C++)
  • text-chunker.ts — Format-aware chunking for markdown/YAML/JSON/TOML/plaintext
  • embedder.ts — nomic-embed-text-v1.5 via @huggingface/transformers with batch processing
  • vector-store.ts — Cosine similarity, vector pack/unpack for SQLite BLOBs
  • fts.ts — FTS5 BM25 search integrated into db.ts
  • poem.ts — POEM/TFPR ranking: vectorized dominance matrix, column duplication weighting
  • query-classifier.ts — Classifies queries as identifier/natural_language/path_like
  • 6 metric modules — BM25, cosine, path-match, symbol-match, import-graph, git-recency
  • index-manager.ts — Index lifecycle: incremental mtime-based updates, embedding orchestration
  • search.ts — Main search API orchestrating all components

Tool definition (src/core/tools/search.ts)

  • Registered in createAllToolDefinitions() / createAllTools()
  • Feature-gated on node:sqlite (Node 22+) — not registered on older Node, no crash
  • Includes promptSnippet, promptGuidelines, renderCall, renderResult

Tests (109 new tests)

  • poem.test.ts — 26 tests (POEM ranking + query classifier)
  • db.test.ts — 26 tests (SQLite, FTS5, embeddings, cascading deletes)
  • text-chunker.test.ts — 24 tests (all 5 text formats)
  • vector-store.test.ts — 12 tests (cosine, pack/unpack, topK)
  • tree-sitter-chunker.test.ts — 11 tests (TS/Python/Go, fallback)
  • search-tool.test.ts — 10 tests (tool registration, schema, execute)

Dependencies added

web-tree-sitter, @huggingface/transformers, tree-sitter grammar packages for 8 languages

Commit: 1af7f1b


Progress tracked by mach6

m-aebrer added 4 commits April 7, 2026 16:11
nomic-embed-text-v1.5 in fp32 was 522MB and caused OOM on indexing.
all-MiniLM-L6-v2 is 23MB, 384-dim, fast native inference via onnxruntime-node.

First-index time for dreb repo: ~3 min (671 files, 6474 chunks).
Subsequent queries: ~6s (includes model load; would be instant with warm model).

Also made prefix handling model-aware (nomic needs search_document:/search_query:
prefixes, most models don't).
The search tool was registered via createAllToolDefinitions() but wasn't
in the hardcoded defaultActiveToolNames array in agent-session.ts.
Now conditionally included when the tool is registered (node:sqlite available).
The actual default tool list is in sdk.ts, not agent-session.ts.
sdk.ts builds initialActiveToolNames from defaultActiveToolNames + alwaysActiveBuiltins,
which is what gets passed to AgentSession. The agent-session.ts list is a fallback
that's only used when no initialActiveToolNames is provided (SDK bypass).
@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Progress Update

Fixes since initial implementation

Model switch: Replaced nomic-embed-text-v1.5 (522MB fp32, OOM on indexing) with all-MiniLM-L6-v2 (23MB, 384-dim). First-index time for dreb repo: ~3 min (671 files, 6474 chunks). Subsequent queries: ~6s including model load.

Tool registration fix: The search tool was being created by createAllToolDefinitions() but never activated. Root cause: sdk.ts builds initialActiveToolNames from its own defaultActiveToolNames + alwaysActiveBuiltins arrays, which is what gets passed to AgentSession. The agent-session.ts default list is a fallback that's only used when no initialActiveToolNames is provided (SDK bypass). Added search to alwaysActiveBuiltins in sdk.ts alongside skill and tasks_update.

Verified with dreb -p: search tool now appears in the active tool list.

Commits:

  • 4072a54 — Switch to all-MiniLM-L6-v2, model-aware prefix handling
  • fd71e3f — Add search to agent-session.ts default active tools
  • 1c44bbc — Make it unconditional (built-in tool, not conditional)
  • f5d625f — Fix actual registration in sdk.ts alwaysActiveBuiltins

Progress tracked by mach6

@m-aebrer m-aebrer marked this pull request as ready for review April 7, 2026 20:35
@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Code Review

Critical

Finding 1: Non-atomic per-file indexing — buildIndex leaves phantom files on partial failure
index-manager.tsupsertFile updates the mtime, then deleteChunksForFile removes old chunks, then chunking/insertion may throw. The catch block swallows the error but the mtime is already committed with zero chunks. Subsequent builds skip the file (mtime matches), making it permanently invisible to search. Fix: wrap the entire per-file operation in db.transaction().

Finding 2: Failed tree-sitter WASM init caches the rejected promise — AST chunking silently disabled forever
tree-sitter-chunker.tsinitPromise holds a rejected promise on WASM init failure; initialized stays false. Every subsequent call returns the same rejection. The caller in chunker.ts catches and silently falls back to plaintext chunking for ALL code files for the rest of the session. Fix: reset initPromise = null in a catch block so retries work.

Finding 3: SQLite connection leaked per search invocation
tools/search.ts (lines 191-196) — A new IndexManager is constructed solely to call getStats(), opening a DatabaseSync connection that is never closed. On a long session with N searches, N file handles leak. Fix: expose getStats() on SearchEngine (which already holds an open connection) and delete the throwaway IndexManager.

Finding 4: computeGitRecencyScores runs one blocking execSync per unique file
metrics/git-recency.ts — For a project with N files, N synchronous git log processes run sequentially (each with 5s timeout), freezing the event loop. A 1000-file project at 50ms/command = 50 seconds of blocking with no progress updates or cancellation. Fix: use a single git log invocation with multiple paths, or batch into a few calls.

Finding 5: getChunksById passes unbounded variadic args — fails on large projects
db.tsWHERE id IN (${placeholders}) with all chunk IDs as positional args. A fresh index on a 5000-file project (~35,000 chunks) exceeds SQLite SQLITE_MAX_VARIABLE_NUMBER (32,766), throwing an unhandled error that aborts embedding. Fix: batch in chunks of ~500 IDs per query.

Important

Finding 6: Model cached per-project instead of shared ~/.dreb/agent/models/
search.ts and index-manager.ts both derive modelCacheDir as <projectRoot>/.dreb/agent/models/. The requirement specifies ~/.dreb/agent/models/ (shared across all projects). Users working across N projects download the model N times.

Finding 7: EMBEDDING_DIMENSION hardcoded at 384 — wrong vectors stored silently for non-default models
embedder.ts — If a model with dimension != 384 is configured, embedDocuments uses wrong stride to slice the output tensor, producing garbage vectors stored silently. embedQuery returns the correct dimension, creating a mismatch. Fix: derive dimension from model config or first batch output shape.

Finding 8: Tree-sitter parser WASM memory leaked when parse() returns null
tree-sitter-chunker.ts — When parser.parse(content) returns null, the function returns early without calling parser.delete(). The finally block (which correctly frees the parser) is only reached when tree is non-null. Fix: move the null-tree early-return inside the try block.

Finding 9: metrics/cosine.ts is unreachable dead code
metrics/cosine.ts exports computeCosineScores but it is never imported anywhere. search.ts computes cosine scores via topKSimilar from vector-store.ts directly. The file creates confusion about which implementation is authoritative. Fix: delete the file or wire it in as the canonical path.

Finding 10: Fragile modelCacheDir derivation via regex in IndexManager
index-manager.ts line 222 — this.config.indexDir.replace(/\.dreb\/index\/?$/, ".dreb/agent/models/"). If indexDir doesn't match the regex, modelCacheDir silently equals indexDir and the ONNX model downloads into the index directory. Fix: use path.join(this.config.projectRoot, ".dreb", "agent", "models").

Finding 11: Home dir scanning would be a recursive nightmare — needs shallow scan mode
scanner.ts — When projectRoot = ~ (common for Telegram/multi-project sessions), the scanner walks the entire home directory. SKIP_DIRS catches node_modules and .git but not arbitrary project directories. Home dirs are typically not git repos so no .gitignore rules apply. Fix: implement a shallow scan mode for home dir that only indexes ~/.dreb/memory/ and top-level files, never recursing into subdirectories. Then project searches can query their own index + the global ~/.dreb/index/ as a secondary source.

Finding 12: GDScript not supported despite being a listed acceptance criterion
scanner.ts, tree-sitter-chunker.ts.gd extension absent from EXTENSION_MAP, GRAMMAR_PATHS, and TreeSitterLanguage. The plan acknowledged this as a risk but never resolved it.

Finding 13: Major test coverage gaps — 4 critical modules entirely untested

  • index-manager.ts (347 lines) — incremental diff logic, import extraction (6 language patterns), path resolution: zero tests
  • search.ts / SearchEngine (304 lines) — sanitizeFtsQuery, pathFilter, aggregation helpers: zero tests
  • scanner.ts (314 lines) — gitignore handling, memory file prefix logic, size gating: zero tests
  • All 6 metric modules (bm25.ts, git-recency.ts, import-graph.ts, path-match.ts, symbol-match.ts, tokenize.ts) — the actual score computation that feeds POEM: zero tests
  • chunker.ts dispatch layer and tree-sitter fallback path: zero tests
  • Tree-sitter chunker tested for 3 of 9 supported languages (missing Rust, Java, C, C++, JS, TSX)

Suggestions

Finding 14: Stale comments in embedder.ts still reference old model/dimensions
File header says "nomic-embed-text-v1.5" and "768-dimensional". Lines 112 and 141 reference "shape [batchLen, 768]". All should reference all-MiniLM-L6-v2 and 384.

Finding 15: Dead YAML preamble code block in text-chunker.ts
Lines ~170-179 compute preambleLines/preambleContent but never use them. The comment says "handled below" and the loop already sets start = 0 for i === 0. Delete the block.

Finding 16: blobToFloat32 in db.ts duplicates unpackVector from vector-store.ts
Byte-for-byte identical implementations. Import and reuse.

Finding 17: Redundant targetNode variable in tree-sitter-chunker.ts
targetNode is initialized to node and only ever reassigned to node again (the if (node.type === "export_statement") block sets targetNode = node). Delete and use node directly.

Finding 18: batchUpsertEmbeddings reimplements transaction() manually
Hand-rolls BEGIN/COMMIT/ROLLBACK pattern. The class already has a transaction() helper that does exactly this.

Finding 19: shouldSkipDir calls toPosix() twice on the same argument
Call once at the top and reuse.

Finding 20: Duplicated comment-lookback logic in TOML chunker
chunkToml has two identical 10-line lookback scan blocks for section && i > 0 and kv && i > 0. Merge into single i > 0 branch.

Strengths

  • Zero native addons — excellent decision to use node:sqlite, web-tree-sitter WASM, and @huggingface/transformers WASM. Keeps dreb's clean dependency story intact.
  • POEM/TFPR ranking is well-implemented — vectorized dominance matrix with column duplication is a smart approach vs hand-tuned weights.
  • Feature gating on Node 22 is clean — tool simply doesn't register on older versions, no crash.
  • SQLite schema design is solid — FTS5 virtual table, proper foreign keys with CASCADE, model-keyed embeddings table supporting future multi-model.
  • AST-aware chunking for 9 languages with format-specific text chunking for 5 text formats is comprehensive.
  • Incremental mtime-based indexing is the right approach for avoiding full re-index on every session.
  • Test coverage for core algorithms (POEM, text chunker, vector store, db, tree-sitter) is thorough where present.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Review Assessment

#113 (comment)

Classifications

Finding Classification Reasoning
1: Non-atomic per-file indexing Genuine Confirmed. upsertFile commits mtime immediately, then chunking may throw. Catch swallows error, leaving file with updated mtime but zero chunks. db.transaction() helper already exists — wrap the sequence.
2: Cached rejected tree-sitter init promise Genuine Confirmed. initPromise holds rejected promise, initialized stays false. Every subsequent call returns same rejection. Silent fallback to plaintext for all code files.
3: SQLite connection leaked per search Genuine Confirmed. New IndexManager → new DatabaseSync opened for getStats(), never closed. Happens every search call.
4: Blocking execSync per file in git-recency Genuine Confirmed. Per-file git log with 5s timeout each. ~10s for typical projects, catastrophic for large ones. Single git invocation with multiple paths is the fix.
5: Unbounded IN clause in getChunksById Genuine Confirmed. 50K-chunk project exceeds SQLite's 32,766 bind variable limit. Batch in groups of ~500.
6: Model cached per-project not shared Genuine Confirmed. Both search.ts and index-manager.ts use <projectRoot>/.dreb/agent/models/. Plan says ~/.dreb/agent/models/. 23MB re-downloaded per project.
7: EMBEDDING_DIMENSION hardcoded at 384 Nitpick Model name is also hardcoded as Xenova/all-MiniLM-L6-v2 which produces 384-dim. Only theoretical if someone changes model without updating constant. Both are compile-time constants that match.
8: Parser WASM leak on null tree Genuine Confirmed. Early return on line ~393 bypasses finally block that calls parser.delete(). WASM heap leak on every failed parse.
9: metrics/cosine.ts is dead code Genuine Confirmed. Zero imports in production code. search.ts uses vector-store.ts directly. Remove or integrate.
10: Fragile modelCacheDir regex Genuine Confirmed. Regex fails silently if indexDir doesn't match pattern. Subsumed by finding 6 fix — derive from homedir() instead.
11: Home dir scanning — needs shallow scan mode Genuine User-identified requirement. When projectRoot = ~, scanner walks entire home dir. Needs shallow scan mode — only index ~/.dreb/memory/ and top-level files, never recurse into subdirectories. Project searches should also query ~/.dreb/index/ as secondary source.
12: GDScript not supported Genuine Plan explicitly lists GDScript as a deliverable. Absent from all relevant maps. Either add support or explicitly descope with a note in the PR.
13: Major test coverage gaps Genuine index-manager.ts, SearchEngine, scanner.ts, all 6 metric modules, chunker.ts dispatch — all zero tests. Per project rules, tests ship with features. At minimum: index-manager (phantom file scenario), scanner (file discovery, gitignore, memory prefix), chunker dispatch (routing + fallback), one metric module (e.g. tokenize.ts as pure function).
14: Stale comments in embedder.ts Nitpick File header says "nomic-embed-text-v1.5" / "768-dimensional" / "~131MB". Misleading but not a bug.
15: Dead YAML preamble block Nitpick Computes preamble, checks non-empty, empty if-body. Preamble correctly handled by start = 0 for first key. Dead code, cosmetic.
16: blobToFloat32 duplicates unpackVector Nitpick Identical 1-liner in two files. Minor DRY violation.
17: Redundant targetNode variable Nitpick targetNode = node, then conditional targetNode = node. No-op assignment, leftover from refactor.
18: batchUpsertEmbeddings reimplements transaction() Nitpick Manual BEGIN/COMMIT/ROLLBACK instead of existing this.transaction(). Functionally identical.
19: shouldSkipDir calls toPosix twice Nitpick Same arg, minor perf waste.
20: Duplicated TOML lookback logic Nitpick Identical 10-line blocks for section and kv kinds. Could extract helper.

Action Plan

  1. Fix non-atomic indexing — Wrap upsert+delete+chunk+insert in db.transaction() in index-manager.ts. Critical data integrity.
  2. Fix cached rejected promise — Reset initPromise = null in catch handler in tree-sitter-chunker.ts.
  3. Fix parser WASM leak — Add parser.delete() before null-tree early return in tree-sitter-chunker.ts.
  4. Fix SQLite connection leak — Expose getStats() on SearchEngine, remove throwaway IndexManager in tools/search.ts.
  5. Batch getChunksById — Split IN clause into groups of ~500 in db.ts.
  6. Fix model cache to global path — Use path.join(homedir(), ".dreb", "agent", "models") in both search.ts and index-manager.ts. Also fixes finding 10.
  7. Optimize git-recency — Replace per-file execSync loop with single git invocation.
  8. Remove dead metrics/cosine.ts — Unreachable code.
  9. Implement home dir shallow scan mode — When projectRoot is the home directory, only scan ~/.dreb/memory/ and top-level files. Project searches should also query ~/.dreb/index/ as a secondary source. User-requested feature.
  10. GDScript — Add support or explicitly descope with a note.
  11. Add tests for untested modules — At minimum: index-manager (phantom file scenario), scanner (file discovery, gitignore, memory prefix), chunker dispatch (routing + fallback), one metric module (e.g. tokenize.ts as pure function).
  12. Clean up nitpicks — Stale comments, dead code blocks, duplicated helpers (findings 14-20). Low priority but easy wins.

Assessment by mach6

… coverage

Critical fixes:
- Wrap per-file indexing in db.transaction() for atomicity (finding 1)
- Reset tree-sitter initPromise on failure to allow retries (finding 2)
- Fix SQLite connection leak in search tool stats (finding 3)
- Replace per-file execSync with single git log call (finding 4)
- Batch getChunksById to avoid SQLite bind variable limit (finding 5)

Metric fixes (all 6 now functional):
- BM25: use OR between terms + stopword removal (was implicit AND)
- Import graph: fix path matching with extension stripping (0/1253 matched)
- Import graph: add self-boost for connected seeds, threshold seed set
- Show all non-zero metrics in results (was limited to top 3)

Important fixes:
- Model cache uses ~/.dreb/agent/models/ (shared, not per-project)
- Fix parser WASM leak on null tree
- Delete dead metrics/cosine.ts
- Add shallow scan mode for home directory
- Add .gitignore entries for .dreb/index/ and .dreb/agent/

Nitpick cleanup:
- Fix stale embedder comments (nomic→MiniLM, 768→384)
- Remove dead YAML preamble block, deduplicate TOML lookback
- Replace blobToFloat32 with shared unpackVector
- Use db.transaction() in batchUpsertEmbeddings
- Remove redundant targetNode variable, deduplicate toPosix call

Tests: +125 new tests (scanner 43, metrics 29, chunker 29, index-manager 24)
@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Progress Update

Review findings fixed + all 6 POEM metrics now functional

16 files changed — 1,812 additions, 220 deletions across 12 source files and 4 new test files.

Critical fixes (findings 1-5)

  • Atomic per-file indexing — wrapped upsert+delete+chunk+insert in db.transaction() so failed files get retried
  • Tree-sitter init retry — reset initPromise on failure so subsequent calls retry instead of returning rejected promise
  • SQLite connection leak — added getStats() to SearchEngine, removed throwaway IndexManager in tool
  • Batched git recency — single git log --name-only call replaces per-file execSync (N calls → 1)
  • Batched getChunksById — groups of 500 to avoid exceeding SQLite bind variable limit (32,766)

Metric fixes — all 6 metrics now producing scores

  • BM25 was dead — FTS5 implicit AND required all terms; switched to OR + stopword removal
  • Import graph was dead (two bugs): import paths stripped extensions but file paths kept them (0/1253 edges matched); seed set included 60%+ of files making propagation useless. Fixed with extension-stripped index + median-threshold seed filtering + self-boost for connected seeds
  • Display showed only top 3 metrics — now shows all non-zero scores

Important fixes (findings 6-12)

  • Model cache path: ~/.dreb/agent/models/ (shared across projects, not per-project)
  • Parser WASM leak on null tree fixed
  • Dead metrics/cosine.ts deleted (unreachable code)
  • Shallow scan mode for home directory (only top-level files, no recursive walk)
  • .gitignore entries for .dreb/index/ and .dreb/agent/
  • GDScript: explicitly descoped (no tree-sitter-gdscript WASM package on npm)

Nitpick cleanup (findings 14-20)

  • Stale embedder comments (nomic→MiniLM, 768→384)
  • Dead YAML preamble block removed, TOML lookback deduplicated
  • blobToFloat32 replaced with shared unpackVector
  • batchUpsertEmbeddings uses db.transaction() helper
  • Redundant targetNode variable removed, toPosix deduped

New tests (+125)

  • scanner.test.ts — 43 tests (file discovery, gitignore, memory prefix, shallow scan)
  • metrics.test.ts — 29 tests (tokenize, path-match, symbol-match, git-recency)
  • chunker.test.ts — 29 tests (dispatch routing, tree-sitter, text chunker, fallback)
  • index-manager.test.ts — 24 tests (lifecycle, incremental builds, atomicity)

Total test count: 210 search tests (85 existing + 125 new), 512 project-wide

Commit: e8e1220


Progress tracked by mach6

@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Code Review

Critical

No critical findings.

Important

Finding 1: EMBEDDING_DIMENSION hardcoded at 384 — silently breaks non-default models
embedder.ts lines 29, 107-112 — EMBEDDING_DIMENSION = 384 is used to slice batch outputs in embedDocuments, but embedQuery returns new Float32Array(output.data) (the full output). The file declares MODEL_PREFIXES for nomic-ai/nomic-embed-text-v1.5 (768-dim), and the JSDoc comment says "384 for all-MiniLM-L6-v2, 768 for nomic." If nomic is configured: stored vectors are truncated to 384 dims, query vectors are 768 dims, cosineSimilarity reads undefined values past index 383, producing NaN for every cosine score. POEM ranking silently degrades. Fix: derive dimension from model output after first embed call (output.data.length / batchLen).

Finding 2: Two separate Embedder instances load the WASM model simultaneously
search.ts line 226 and index-manager.ts line 223 — SearchEngine and IndexManager each create their own Embedder for the same model. During search(): indexManager.buildIndex() initializes one embedder, then SearchEngine.computeVectorScores() initializes a second. Both stay alive indefinitely. Each ONNX session holds ~23MB of model weights plus WASM overhead — doubled for no reason. Fix: share the embedder between the two.

Finding 3: execSync in computeGitRecencyScores blocks the event loop
metrics/git-recency.ts lines 96-120 — git log without --max-count or --since traverses the full commit history synchronously with a 15-second timeout and 10MB buffer. For a large/old repo this blocks the event loop for seconds. All other metrics are either pure JS or async. Fix: switch to async execFile and make computeGitRecencyScores return a Promise.

Finding 4: Bare catch {} in buildIndex silently swallows systemic WASM failures
index-manager.ts lines 119-135 — When tree-sitter WASM init fails (e.g., OOM), the chunker falls back to plaintext, and if that also fails, the outer catch silently skips the file. The developer sees { added: 500, updated: 0, removed: 0 } — a completely normal result — but all 500 TypeScript files were indexed without AST structure. There is no counter for skipped files, no onProgress error phase, no observable signal. Fix: count and surface skipped files; distinguish systemic failures (WASM init) from per-file failures.

Finding 5: engineCache leaks SQLite connections and ONNX sessions for process lifetime
tools/search.ts lines 72-80 — Module-level engineCache retains a SearchEngine per unique cwd, each owning a DatabaseSync connection (3 open file handles in WAL mode) and an ONNX session. engine.close() is never called. Long-running sessions that switch cwd accumulate resources permanently. Fix: register a process exit handler to close all cached engines.

Finding 6: Documentation not updated for the new search tool
Per project rules, docs must ship in the same PR as behavior changes. The following locations still reflect the pre-PR state:

  • README.md line 69: "All 10 built-in tools" should be 11, search not listed
  • README.md line 537: available tool list missing search
  • docs/extensions.md line 1477: built-in tools list missing search
  • sdk.ts line 65 JSDoc: default tools list missing search

Finding 7: Major test coverage gaps — orchestration layer and 2 of 6 metrics untested

  • computeImportGraphScores (132 lines, already had a bug this PR) — zero tests
  • sanitizeFtsQuery (private, specifically fixed in this PR for BM25 OR semantics) — zero tests
  • extractImports (80 lines, 7-language regex parser, source of prior bug) — zero tests
  • SearchEngine.search() pipeline (398 lines, main orchestrator) — only empty-query test; no integration test
  • computeBm25Scores normalization wrapper — zero tests
  • Shallow home-directory scan mode (scanShallow) — zero tests
  • getChunksById batch-boundary logic (specifically added to fix prior finding 5) — zero tests

Suggestions

Finding 8: sanitizeFtsQuery returns '""' for all-stopword queries — FTS5 behavior is SQLite-version-dependent
search.ts line 233 — When all query tokens are stopwords (e.g., "where is the"), returns the string "" as an FTS5 MATCH argument. Depending on the bundled SQLite version, this may match all rows (flooding BM25 with noise) or throw (silently caught). Fix: return an empty string and short-circuit computeBm25Scores.

Finding 9: Global memory index architecture diverges from spec — no ~/.dreb/index/
The plan specifies separate project and global memory indexes. Implementation stores global memory files in the project index with a ~memory/ path prefix. The user-facing capability works, but global memory is re-indexed per project and wiping a project index also loses its memory embeddings.

Finding 10: ~memory/ path prefix collides with literal project directories named ~memory
scanner.ts + index-manager.ts — A project file at <root>/~memory/notes.md gets routed to ~/.dreb/memory/notes.md for content reading, silently indexing wrong content. Fix: use a collision-resistant prefix or separate the result sets before merging.

Finding 11: Dynamic await import("./vector-store.js") inside computeVectorScores
search.ts line 243 — vector-store.ts is a pure utility module with no I/O or initialization side effects. Every other metric helper is statically imported. Fix: use a static import.

Finding 12: Dead parameter _sourceLines in extractRegions
tree-sitter-chunker.ts line 243 — The underscore-prefixed parameter is never used. Remove it.

Finding 13: computeBm25Scores outer try/catch is unreachable
bm25.ts lines 14-31 — db.ftsSearch already has its own internal try/catch returning [] on failure. The outer catch can never trigger.

Finding 14: db.getAllFiles() called redundantly in computeImportGraphScores
import-graph.ts line 33 — File lookup tables could be derived from allChunks already in scope at the call site, avoiding a second DB query.

Finding 15: upsertFile uses SELECT + conditional UPDATE/INSERT instead of SQL upsert
db.ts lines 166-179 — SQLite 3.24+ supports INSERT ... ON CONFLICT DO UPDATE ... RETURNING id, collapsing to one statement per file.

Finding 16: getIndexConfig() called twice per search invocation
search.ts — Called in both getIndexManager() and computeVectorScores(). Config values are immutable. Cache at construction time.

Strengths

  • Zero native addonsnode:sqlite, web-tree-sitter WASM, and @huggingface/transformers WASM keep dreb's dependency story clean
  • POEM/TFPR ranking with vectorized dominance matrix and column duplication is well-implemented — avoids hand-tuned weights
  • Feature gating on Node 22 is clean — tool simply does not register on older versions
  • SQLite schema design is solid — FTS5 with BM25 weights, proper foreign keys with CASCADE, model-keyed embeddings
  • Atomic per-file indexing (fixed in this cycle) — transaction wrapping prevents phantom files
  • Tree-sitter init retry (fixed in this cycle) — initPromise reset on failure enables recovery
  • Incremental mtime-based indexing avoids full re-index on every session
  • Test coverage for core algorithms (POEM, text chunker, vector store, db, tree-sitter, scanner) is thorough where present

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Review Assessment

#113 (comment)

Classifications

Finding Classification Reasoning
1: EMBEDDING_DIMENSION hardcoded at 384 Genuine (defer) Real bug for non-default models — stored vectors truncated, query vectors full-length, cosine produces NaN. But only default model (384-dim) is used; no UI to configure nomic. Defer until model selection is exposed.
2: Two Embedder instances Genuine (defer) Confirmed: IndexManager and SearchEngine each load the same ONNX model. ~46MB vs ~23MB. Design consequence of separated indexing/query paths. Not a correctness issue; acceptable overhead for current model size.
3: execSync blocks event loop Genuine git log with no --max-count traverses full history synchronously. 15s timeout caps worst case but multi-second blocks on large repos are a real TUI UX problem. Add --max-count to bound the block.
4: Bare catch swallows WASM failures False positive chunker.ts has its own try/catch that falls back to plaintext on tree-sitter failure. The outer catch in buildIndex only triggers for I/O errors (permissions, deleted files) — exactly what should be silently skipped.
5: engineCache leaks connections Genuine (defer) Module-level cache never calls engine.close(). For a CLI that exits after sessions, OS reclaims resources. For persistent processes, this accumulates. Low risk for current use case.
6: Docs not updated Genuine — must fix README.md (2 locations), docs/extensions.md (2 locations), sdk.ts JSDoc all list 10 tools without search. Project policy: "Docs must ship in the same PR as behavior changes."
7: Test coverage gaps Genuine — must fix Confirmed zero tests for: computeImportGraphScores (132 lines, had a bug this PR), computeBm25Scores wrapper, SearchEngine.search() pipeline (only empty-query test), getChunksById batch logic (added to fix prior finding). Project policy: "Tests ship with features, not deferred."
8: sanitizeFtsQuery returns '""' False positive Traced both paths: if FTS5 throws on "" → caught by ftsSearch, returns []. If it matches rows with score 0 → maxScore === 0 guard returns empty map. Both paths produce correct empty BM25 scores.
9: Architecture diverges from spec False positive Per project memory: "Spec is a living doc, features override it." Global memory stored in project index with ~memory/ prefix is a valid simplification. The user-facing capability (memory files searchable) works correctly.
10: ~memory/ path collision Genuine (defer) Real collision: project dir named ~memory would route to wrong content. Probability essentially zero in practice. Add a comment documenting the assumption.
11: Dynamic import of vector-store Genuine (easy fix) vector-store.ts is a pure utility already statically imported by db.ts. Dynamic import adds unnecessary async overhead per search. One-line fix.
12: Dead _sourceLines parameter Nitpick Underscore convention makes it explicit. No correctness impact.
13: Unreachable catch in bm25 Nitpick db.ftsSearch already catches internally. Dead code, not harmful.
14: Redundant db.getAllFiles() Nitpick Could derive file maps from allChunks. Minor extra DB query, zero correctness impact.
15: upsertFile uses SELECT + UPDATE/INSERT Nitpick Works correctly under single-connection WAL. SQL upsert would be cleaner but not necessary.
16: getIndexConfig() called twice Nitpick Microsecond cost, pure function. Harmless.

Action Plan

Must fix before merge:

  1. Update all documentation (finding 6) — Change "10" to "11" built-in tools, add search to tool lists in README.md (2 locations), docs/extensions.md (2 locations), sdk.ts JSDoc
  2. Add missing tests (finding 7) — At minimum: computeImportGraphScores (propagation, empty seeds), computeBm25Scores (normalization), SearchEngine.search() integration (mock embedder, fixture project), getChunksById batch boundary (>500 IDs)

Should fix in this PR:

  1. Cap git log (finding 3) — Add --max-count 10000 to the git log command in git-recency.ts to bound the event-loop block
  2. Static import (finding 11) — Move import { topKSimilar } from "./vector-store.js" to top of search.ts

Defer (create issues):

  1. Finding 1 — Non-default model dimension support (when model selection is exposed)
  2. Finding 2 — Shared Embedder instance (design refactor, ~23MB overhead acceptable)
  3. Finding 5 — engineCache cleanup (CLI exits handle this; track for persistent process use)
  4. Finding 10 — ~memory/ collision-resistant prefix (probability ~zero)

Assessment by mach6

- Update docs: 10→11 built-in tools, add search to all tool lists
  (README.md, extensions.md, sdk.ts JSDoc)
- Cap git log with --max-count=10000 in git-recency metric
- Replace dynamic import with static import for vector-store in search.ts
- Add tests for computeImportGraphScores (8), computeBm25Scores (5),
  getChunksById batching (7), SearchEngine.search() integration (11)
@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Progress Update

Review round 2 fixes — docs, tests, git-recency cap, static import

8 files changed — 678 additions, 10 deletions.

Documentation (finding 6)

  • README.md: "10 built-in tools" → "11 built-in tools", added search to tool list (2 locations)
  • docs/extensions.md: added search to tool name comment and overridable tools list (2 locations)
  • src/core/sdk.ts: updated JSDoc to include search in default tools

Bug fixes

  • Git recency cap (finding 3): added --max-count=10000 to git log in git-recency.ts to bound event-loop blocking
  • Static import (finding 11): replaced dynamic await import("./vector-store.js") with static import in search.ts

New tests — 31 tests added (241 total search tests, 512 project-wide)

  • metrics.test.ts (+13): computeImportGraphScores (8 tests — propagation, self-boost, normalization, extension-stripped paths), computeBm25Scores (5 tests — normalization, proportional scores, edge cases)
  • db.test.ts (+7): getChunksById batching (empty input, valid/invalid IDs, 500-ID boundary, 501-ID cross-batch, 1000+ IDs, correct shape)
  • search-engine.test.ts (new file, 11 tests): end-to-end SearchEngine integration with mock embedder — identifier/NL/path queries, limit, pathFilter, result shape, index reuse

Commit: 4883ac3


Progress tracked by mach6

@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Code Review (Round 3)

Critical

No critical findings.

Important

Finding 1: EMBEDDING_DIMENSION hardcoded to 384 silently corrupts embeddings for non-default models
embedder.ts lines 29, 113-116 — EMBEDDING_DIMENSION = 384 slices the flat output buffer in embedDocuments, but MODEL_PREFIXES registers nomic-embed-text-v1.5 (768-dim) as a supported model. For a 768-dim batch: doc 1 gets first half of doc 0, doc 2 gets second half of doc 0, etc. Meanwhile embedQuery returns the full unsliced tensor. Stored 384-dim garbage vs 768-dim query → NaN cosine scores. Fix: derive dimension from output.data.length / batchLen after first embed call.

Finding 2: SearchEngine and IndexManager each create separate Embedder instances, loading the model twice
search.ts line 51+225 and index-manager.ts line 31+221 — Both independently guard-and-create an Embedder for the same model. On first search, pipeline(...) invoked twice, loading ONNX into two WASM runtimes. Doubles init time and memory (~46MB vs ~23MB). Fix: share the embedder between them.

Finding 3: computeGitRecencyScores calls execSync synchronously, blocking the event loop
metrics/git-recency.ts line 96 — git log --max-count=10000 with 15s timeout runs synchronously. Multi-second blocks on large repos freeze TUI rendering. All other metrics are pure JS or async SQL. Fix: switch to async execFile/spawn.

Finding 4: scanMemoryDir loses subdirectory path components, causing silent indexing failures
scanner.ts lines 234-276 — Recursive call passes fullPath as new memoryDir, so relative(memoryDir, fullPath) for nested files produces paths relative to the subdirectory, not the root. File at ~/.dreb/memory/subdir/foo.md gets path ~memory/foo.md instead of ~memory/subdir/foo.md. In buildIndex, readFileSync resolves to wrong path → ENOENT → swallowed by catch. Fix: pass a stable baseMemoryDir through recursion.

Finding 5: buildIndex bare catch {} swallows DB-level errors and returns misleading operation counts
index-manager.ts lines 100-148 — SQLite errors (disk full, WAL corruption) during per-file transactions are caught alongside file-level errors (EACCES, ENOENT). Return value reflects planned operations, not successful ones. Disk-full scenario: every file after failure point silently fails, returns { added: 50 } when 0 indexed. Fix: re-throw DB-level errors; track and surface failed count.

Finding 6: Broken #semantic-search anchor and contradictory tool placement in README
README.md line 540 — [semantic codebase search](#semantic-search) links to a nonexistent heading. Additionally, search appears both in "Available built-in tools" (implying --tools controls it) and under "always active" tools — contradictory, since code puts it in alwaysActiveBuiltins. Fix: add a Semantic Search section, remove from the --tools-controllable list.

Finding 7: Search tool execute() path almost entirely untested
search-tool.test.ts — Only one test calls execute() (empty-query guard). Entire result-producing path (formatting, scores, content preview, truncation, indexStats footer) never exercised. A formatting bug would be invisible.

Finding 8: Six of nine tree-sitter languages completely untested
tree-sitter-chunker.test.ts — Covers TS, Python, Go, and fallback. Missing: TSX, JavaScript, Rust, Java, C, C++. Each has distinct extractor node types. A tree-sitter grammar node type mismatch would silently degrade to whole-file chunks.

Finding 9: scanShallow() (home directory scan mode) never tested
scanner.ts — When projectRoot equals home dir, scanShallow() runs instead of walkDirectory(). Different filtering logic, no recursion, skips dotfiles. Zero tests cover this path.

Finding 10: File update path does not verify stale chunk/embedding elimination
index-manager.test.ts — "detects mtime changes" test asserts result.updated === 1 but never checks old chunks are gone. If deleteChunksForFile had a bug, duplicate chunks accumulate silently on every update.

Finding 11: extractImports (7-language regex parser) never tested end-to-end
index-manager.ts — Import graph metric tests use manual db.insertImport(). The actual regex-based extractImports function and resolveImportPath/resolvePythonImport helpers are never verified to produce correct edges during buildIndex.

Suggestions

Finding 12: Gap chunks (collectGaps) between extracted regions never verified in tests
Tree-sitter tests only assert named constructs. Module-level code between declarations (imports, constants) would become invisible to search if collectGaps broke.

Finding 13: scannedByPath should be a Set, not a Map
index-manager.ts ~line 78 — Map values never accessed, only .has() called. Set<string> communicates intent.

Finding 14: existingFile guard around deletes is unnecessary
index-manager.ts ~line 135 — upsertFile returns authoritative fileId. DELETE on a new file is a safe no-op.

Finding 15: Dead _sourceLines parameter in extractRegions
tree-sitter-chunker.ts ~line 218 — Underscore-prefixed, never used. Remove from signature and call sites.

Finding 16: Redundant !Parser in init guard
tree-sitter-chunker.ts ~line 271 — initialized is only true after Parser is assigned. !Parser can never be true when initialized is true.

Finding 17: getIndexConfig() called just to read a module-level constant
search.ts ~line 167 — config.modelName is always DEFAULT_MODEL_NAME. Use the constant directly.

Finding 18: ?? 0 fallbacks on non-optional MetricScores fields
poem.ts ~line 196 — All six fields typed as number (not optional). Fallbacks are dead code.

Finding 19: Module-level engineCache holds open resources for process lifetime
tools/search.ts lines 97-104 — SearchEngine.close() never called on cached instances. Each holds SQLite WAL connection + loaded ONNX model. Low risk for CLI (single cwd, process exit reclaims), but accumulates in multi-project SDK contexts. Fix: expose closeAll() and hook into session teardown.

Strengths

  • Zero native addonsnode:sqlite, web-tree-sitter WASM, @huggingface/transformers WASM keeps the clean dependency story intact
  • POEM/TFPR ranking with vectorized dominance matrix and column duplication is a smart approach vs hand-tuned weights
  • Feature gating on Node 22 — tool simply does not register on older Node, no crash
  • Prior review fixes — atomic indexing, init retry, connection leak, batched operations, model cache path all properly addressed
  • Comprehensive test coverage where present — POEM, text chunker, vector store, DB, scanner gitignore, SearchEngine integration
  • Incremental mtime-based indexing avoids full re-index on every session

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Review Assessment

#113 (comment)

Classifications

Finding Classification Reasoning
1: EMBEDDING_DIMENSION hardcoded to 384 Genuine Confirmed. embedDocuments slices at 384, embedQuery returns full tensor. MODEL_PREFIXES registers nomic (768-dim). Latent since only default model used, but code was written to support nomic — silent corruption if used.
2: Dual Embedder instances Genuine Confirmed. SearchEngine and IndexManager each load the ONNX model independently. Doubles ~23MB memory and ~10s init time on first search.
3: execSync blocks event loop Genuine Confirmed. git log --max-count=10000 with 15s timeout runs synchronously. All other metrics are pure JS or async SQL. Multi-second TUI freeze on large repos.
4: scanMemoryDir loses subdirectory paths Genuine Confirmed. Recursive call passes fullPath as new memoryDir, so relative() computes from subdirectory, not root. ~/.dreb/memory/subdir/foo.md gets path ~memory/foo.md → ENOENT in buildIndex. Existing test documents the bug with a comment: "nested files appear with just their filename."
5: buildIndex bare catch swallows DB errors Genuine Confirmed. catch {} catches both expected file errors and systemic SQLite errors. Return value reports planned operations, not successful ones. Disk-full scenario silently skips all files.
6: Broken README anchor and contradictory placement Genuine Confirmed. No ## Semantic Search heading exists. search listed both in --tools-controllable list and always-active list — contradictory since code puts it in alwaysActiveBuiltins.
7: Search tool execute() mostly untested Genuine Confirmed. One execute() test (empty query guard). Zero tests for result formatting, scores, content preview, truncation, indexStats footer.
8: Six tree-sitter languages untested Genuine Confirmed. TSX, JavaScript, Rust, Java, C, C++ all have distinct NodeExtractor arrays (e.g., Rust impl_item/enum_item, C cFunctionName, C++ class_specifier). None tested.
9: scanShallow() never tested Genuine Confirmed. Zero tests for isHomeDirPathscanShallow path. Distinct filtering logic (skips dotfiles, no recursion, no .gitignore).
10: File update stale chunk elimination unverified Genuine Confirmed. Test asserts result.updated === 1 but never checks old chunks deleted and new chunks match updated content.
11: extractImports never tested end-to-end Genuine Confirmed. grep for extractImport in test/ returned no matches. All import graph tests use manual db.insertImport(). 7-language regex parser completely untested.
12: Gap chunks never verified Nitpick True but lower impact. collectGaps is simple arithmetic over sorted regions.
13: scannedByPath Map → Set Nitpick Only .has() called on values. Trivial efficiency improvement, no correctness impact.
14: existingFile guard unnecessary False positive Guard IS necessary. filesToProcess includes both toAdd and toUpdate files. For new files, existingByPath.get() returns undefined — without guard, deleteChunksForFile(undefined) would be called.
15: Dead _sourceLines parameter Nitpick Underscore prefix is the TS convention for intentionally unused. No correctness impact.
16: Redundant !Parser in init guard Nitpick Defensive check, minor redundancy.
17: getIndexConfig() for a constant Nitpick Minor inefficiency, aids future extensibility.
18: ?? 0 on non-optional fields False positive candidateIds is union of all score map keys. A chunk in bm25Scores might not be in cosineScores.get(id) returns undefined. Fallback IS necessary despite the type declaration.
19: engineCache holds resources Nitpick Standard CLI practice. Process exit reclaims everything. Only matters in long-running server context.

Action Plan

Correctness bugs (fix first):

  1. Fix scanMemoryDir subdirectory path loss (finding 4) — Pass original memoryDir root through recursion via baseMemoryDir parameter. Update existing test to expect ~memory/sub/deep.md instead of ~memory/deep.md.

  2. Fix EMBEDDING_DIMENSION hardcoding (finding 1) — Derive dimension from output.data.length / batchLen after first embed call. Remove or make dimension getter dynamic.

  3. Fix bare catch {} in buildIndex (finding 5) — Distinguish file-level errors (skip) from DB errors (re-throw). Track and surface failed count.

Resource/performance:

  1. Share Embedder between SearchEngine and IndexManager (finding 2) — Pass embedder from SearchEngine to IndexManager. Eliminates double model load (~10s savings).

  2. Switch git-recency to async (finding 3) — Replace execSync with async execFile/spawn. Prevents multi-second event loop blocking.

Documentation:

  1. Fix README (finding 6) — Remove search from --tools-controllable list, add Semantic Search section, fix broken anchor.

Test coverage:

  1. extractImports end-to-end tests (finding 11)
  2. Tree-sitter tests for 6 missing languages (finding 8)
  3. Search tool execute() result path tests (finding 7)
  4. File update stale-chunk verification (finding 10)
  5. scanShallow() tests (finding 9)

Assessment by mach6

@m-aebrer
Copy link
Copy Markdown
Collaborator Author

m-aebrer commented Apr 7, 2026

Progress Update

Review round 3 fixes — 5 bugs, docs, 35 new tests

12 files changed — 770 additions, 85 deletions across 7 source files and 5 test files.

Bug fixes (findings 1-5)

  • Embedder dimension fix — Removed hardcoded EMBEDDING_DIMENSION = 384. Now derives dimension from model output shape (output.data.length / batchLen), cached after first embed. Fixes silent corruption for non-384-dim models (e.g., nomic-embed-text-v1.5 at 768-dim). embedQuery also slices consistently.
  • Shared EmbedderSearchEngine and IndexManager now share a single Embedder instance via setEmbedder(). Eliminates double ONNX model load (~23MB memory saved, ~10s init time saved).
  • Async git-recency — Replaced execSync with promisified execFile in computeGitRecencyScores. No longer blocks the event loop for up to 15s on large repos. TUI stays responsive during git operations.
  • scanMemoryDir path fix — Added baseMemoryDir parameter through recursion so nested memory files get correct paths (~memory/subdir/foo.md instead of ~memory/foo.md). Fixes ENOENT on nested global memory files.
  • buildIndex error handling — Bare catch {} now distinguishes file-level errors (skip, continue) from DB-level errors (re-throw SQLITE errors). Added failed counter to return value.

Documentation (finding 6)

  • README: Fixed tool count (10 controllable + 3 always-active), removed search from --tools list, added ## Semantic Search section with feature description, ToC entry, fixed broken #semantic-search anchor
  • sdk.ts: Updated JSDoc to remove search from default tools list

New tests — 35 tests added (276 search tests, 547 project-wide)

  • search-tool.test.ts (+6): execute() result formatting, scores, truncation, details, no-results
  • tree-sitter-chunker.test.ts (+16): TSX, JavaScript, Rust, Java, C, C++ language coverage
  • scanner.test.ts (+8): scanShallow home-dir mode, dotfile skipping, no recursion, file type mapping
  • index-manager.test.ts (+5): stale chunk elimination on update, TypeScript + Python extractImports e2e

Commit: e175bfe


Progress tracked by mach6

@m-aebrer m-aebrer merged commit 47598ec into master Apr 7, 2026
2 checks passed
@m-aebrer m-aebrer deleted the feature/issue-108-semantic-codebase-search branch April 7, 2026 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Semantic codebase search

1 participant