Skip to content

docs(roadmap): plan full rusqlite DB migration (Phase 6.13–6.17)#652

Merged
carlos-alm merged 4 commits intomainfrom
docs/roadmap-rusqlite-migration
Mar 27, 2026
Merged

docs(roadmap): plan full rusqlite DB migration (Phase 6.13–6.17)#652
carlos-alm merged 4 commits intomainfrom
docs/roadmap-rusqlite-migration

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • Adds 5 new roadmap phases (6.13–6.17) planning the complete migration of all DB operations to rusqlite on the native engine path
  • Goal: better-sqlite3 becomes WASM-only, rusqlite becomes native-only — eliminating the dual-SQLite-in-one-process problem
  • Updates Phase 6 overview count from 12 to 17 items

Phases

  • 6.13NativeDatabase napi-rs class with connection lifecycle
  • 6.14 — Migrate all 41 Repository read methods to Rust
  • 6.15 — Migrate all build-pipeline write operations to Rust
  • 6.16 — Dynamic SQL (NodeQuery builder) and edge cases
  • 6.17 — Cleanup, better-sqlite3 lazy-loading isolation, README update (3→4 deps)

Context

PR #651 introduced rusqlite for bulk AST node insertion, but better-sqlite3 still handles all other DB operations for both engines. This roadmap plans the full migration so the native engine never touches better-sqlite3.

Test plan

  • Verify roadmap renders correctly on GitHub
  • Confirm phase numbering is sequential and consistent

Plan the complete migration of all DB operations to rusqlite on the
native engine path, so better-sqlite3 is only used for WASM fallback:

- 6.13: NativeDatabase class (connection lifecycle)
- 6.14: Native read queries (41 Repository methods)
- 6.15: Native write operations (build pipeline)
- 6.16: Dynamic SQL & edge cases
- 6.17: Cleanup & better-sqlite3 isolation
@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude finished @carlos-alm's task —— View job


PR Review: Roadmap Documentation for Rusqlite Migration

Todo List:

  • Read and analyze the modified ROADMAP.md file
  • Apply 5-phase review methodology
  • Assess backlog compliance criteria
  • Provide structured review feedback with rating

Root Cause Analysis

Problem Being Solved: The current architecture has both native and WASM engines using better-sqlite3 for DB operations, creating a "dual-SQLite-in-one-process problem" where the native engine could benefit from pure Rust rusqlite operations for better performance. PR #651 introduced rusqlite for bulk AST insertion, but all other DB operations still go through better-sqlite3.

Why This Approach: The planned migration makes architectural sense - segregating DB engines by runtime path (native → rusqlite, WASM → better-sqlite3) would eliminate the dual-connection complexity and unlock Rust-speed for all DB operations on the native path. The phased approach (6.13-6.17) is methodical and reduces risk.

Risk Assessment: Low risk - this is documentation-only planning. The actual implementation will require careful parity testing between engines, but the roadmap properly identifies this need.

Backlog Compliance

  • Zero-dep: ✓ — No new runtime dependencies; rusqlite is already introduced in PR perf(ast): bulk-insert AST nodes via native Rust/rusqlite #651
  • Foundation-aligned: ✓ — Aligns with P2 (native speed, universal reach) by optimizing native engine performance while maintaining WASM fallback
  • Problem-fit: 3 — Improves performance for native engine users but doesn't directly address the core "lost AI agents" problem; more of an optimization than a navigation/context enhancement
  • Breaking: No — All changes are internal architecture; CLI/API contracts remain unchanged
  • Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Assessment

Phase 1: Root Cause Analysis ✅

This PR addresses a legitimate architectural concern identified after PR #651. The problem is real - having both SQLite engines in the same process is suboptimal for the native path.

Phase 2: Configuration Change Detection ✅

No configuration changes - this is pure documentation.

Phase 3: Test Integrity ✅

No test changes - documentation only.

Phase 4: Code Quality ✅

Documentation is well-structured, detailed, and follows existing roadmap patterns. The phase breakdown is logical:

  • 6.13: Foundation (connection lifecycle)
  • 6.14: Read operations migration
  • 6.15: Write operations migration
  • 6.16: Edge cases and dynamic SQL
  • 6.17: Cleanup and isolation

Phase 5: Backlog Compliance ✅

Properly updates phase count from "7 of 12 done" to "7 of 17 done" to reflect new scope.

Critical Concerns

None - this is solid planning documentation.

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: Well-planned architectural improvement with clear phases, proper scope expansion documentation, and no breaking changes. Loses one star only because it's an internal optimization rather than a user-facing feature that directly addresses the core AI navigation problem.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR adds five new roadmap sub-phases (6.13–6.17) to docs/roadmap/ROADMAP.md, planning the full migration of all DB operations from better-sqlite3 to rusqlite on the native engine path. The Phase 6 overview item count is correctly updated from 12 to 17. The previously flagged incorrect "3 → 4 runtime deps" claim in 6.17 has been fixed — the file now explicitly states the dep count stays at 3 since rusqlite is compiled into the native addon.\n\n- Phase 6.13 defines the NativeDatabase napi-rs class as the foundation for all subsequent migrations\n- Phase 6.14 plans migrating all 41 Repository read methods with a parity test suite\n- Phase 6.15 covers all build-pipeline write operations and consolidates scattered rusqlite usage\n- Phase 6.16 handles NodeQuery dynamic SQL edge cases and miscellaneous DB patterns\n- Phase 6.17 is the cleanup phase: lazy-loading better-sqlite3, removing the double-connection pattern, and updating README — dep count correctly noted as unchanged\n\nOne minor overlap was identified: both 6.15 and 6.17 describe consolidating bulk_insert_ast_nodes into the shared NativeDatabase connection, which could confuse the implementer about when that work actually happens.

Confidence Score: 5/5

Docs-only roadmap change; no code modified; prior dep-count concern resolved; safe to merge.

The only changed file is a roadmap Markdown document. The previously raised concern about the incorrect '3→4 deps' claim has been addressed in the latest commit. The single remaining note (task duplication between 6.15 and 6.17) is editorial and does not block merging. Phase numbering is sequential and internally consistent.

No files require special attention — this is a documentation-only PR.

Important Files Changed

Filename Overview
docs/roadmap/ROADMAP.md Adds phases 6.13–6.17 detailing the full rusqlite migration plan; overview count updated from 12 to 17; dep-count claim in 6.17 correctly fixed to keep the count at 3; minor overlap between 6.15 and 6.17 on bulk_insert_ast_nodes consolidation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["6.13 — NativeDatabase class\n(rusqlite connection lifecycle)"]
    B["6.14 — Native Read Queries\n(41 Repository methods → Rust)"]
    C["6.15 — Native Write Operations\n(build-pipeline writes → Rust)"]
    D["6.16 — Dynamic SQL & Edge Cases\n(NodeQuery builder, WAL, advisory locks)"]
    E["6.17 — Cleanup & Isolation\nbetter-sqlite3 lazy-load, README update"]

    A --> B
    A --> C
    B --> D
    C --> D
    D --> E

    subgraph result["End state"]
        F["Native engine → rusqlite only"]
        G["WASM engine → better-sqlite3 only"]
    end
    E --> result
Loading

Reviews (2): Last reviewed commit: "fix(roadmap): correct dep count claim in..." | Re-trigger Greptile

- **Lazy-load `better-sqlite3`** — only `require()` it when engine is WASM; native path never touches it
- **Remove the double-connection pattern** — `bulk_insert_ast_nodes` (6.9) and any other Rust functions that open their own `rusqlite::Connection` should use the shared `NativeDatabase` instance
- **Profile and tune:** Enable `rusqlite` statement caching, optimize batch sizes, tune WAL settings for the unified Rust connection
- **Update README.md** — change "3 runtime dependencies" to "4 runtime dependencies" with the claim: `better-sqlite3` is WASM-engine only, `rusqlite` is native-engine only
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 "4 runtime dependencies" claim may not hold

The plan says to bump the README from "3 runtime dependencies" to "4 runtime dependencies," citing rusqlite as a new dep. However, rusqlite is a Rust crate compiled into the native addon binary — it's not a separate npm runtime dependency visible to users. From an npm perspective the count would stay the same (or even drop by one if better-sqlite3 becomes optional/conditional on WASM).

A more accurate framing might be: "The project's SQLite backend is now split — WASM engine uses better-sqlite3 (npm), native engine uses rusqlite (compiled into the addon)" — without changing the raw dependency count claim unless you can verify that the count actually changes.

It's worth pinning down the exact README wording in this phase's acceptance criteria so the implementer doesn't have to re-evaluate it then.

Context Used: CLAUDE.md (source)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 82492b8. Removed the "3 → 4" dep count bump since rusqlite is compiled into the native addon binary, not a separate npm runtime dependency. The bullet now reads: clarify that better-sqlite3 is WASM-engine only, and the dep count stays at 3.

@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm carlos-alm merged commit 0fa8053 into main Mar 27, 2026
12 checks passed
@carlos-alm carlos-alm deleted the docs/roadmap-rusqlite-migration branch March 27, 2026 05:51
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant