Skip to content

feat: sqlite#3666

Closed
NathanFlurry wants to merge 1 commit into02-03-feat_use_casesfrom
12-20-feat_sqlite
Closed

feat: sqlite#3666
NathanFlurry wants to merge 1 commit into02-03-feat_use_casesfrom
12-20-feat_sqlite

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
rivetkit-serverless Error Error Jan 2, 2026 8:02pm
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 2, 2026 8:02pm
rivet-inspector Ignored Ignored Preview Jan 2, 2026 8:02pm
rivet-site Ignored Ignored Preview Jan 2, 2026 8:02pm

Copy link
Member Author

NathanFlurry commented Dec 21, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Dec 21, 2025

Pull Request Review: SQLite Support

This PR adds SQLite database support to RivetKit with a custom KV-backed VFS implementation.

✅ Strengths

  1. Solid Architecture - Clean separation between VFS layer and database providers with well-designed DatabaseProvider abstraction
  2. Good Test Coverage - Comprehensive VFS tests and driver integration tests
  3. Documentation - Examples follow template with clear READMEs

⚠️ Critical Issues

1. SQL Injection Vulnerability (rivetkit-typescript/packages/db/src/raw/mod.ts:36-38, 64-65)

The raw database wrapper accepts parameterized queries but doesn't bind them. The exec method only takes a callback, so parameters are ignored. This is misleading and potentially unsafe.

Fix: Either implement proper parameter binding or remove the args parameter.

2. Missing Error Handling in AsyncMutex (rivetkit-typescript/packages/sqlite-vfs/src/mod.ts:105-123)

If an error occurs between acquire and release, the mutex stays locked forever.

Fix: Add timeout support or document that callers MUST use try/finally.

High Priority Issues

3. Query Type Detection (rivetkit-typescript/packages/db/src/raw/mod.ts:45-63)

String-based query type detection is fragile and doesn't handle comments or CTEs.

4. Parameter Validation - getChunkKey and getMetaKey should validate inputs

5. Performance - Array operations could be optimized for large reads

Medium Priority

6. Incomplete Cleanup - Database cleanup might not handle all edge cases (failed close, journal files, KV cleanup)

7. Magic Numbers - CHUNK_SIZE and prefix bytes should be named constants

8. Code Duplication - Similar execute wrappers in multiple files

Security Concerns

  1. Parameter binding doesn't work (SQL injection risk)
  2. File paths should be validated (directory traversal)
  3. Error messages might expose schema info

Test Coverage

Good: CRUD, persistence, transactions, large data, multiple databases, actor isolation

Missing: Error conditions, concurrent access, migration failures, cleanup, parameter binding, SQL injection tests

Action Items

Must Fix:

  1. Fix or document parameter binding issue
  2. Add error handling docs for AsyncMutex
  3. Validate file paths

Should Fix:
4. Improve query type detection
5. Add error case tests
6. Document cleanup behavior

Summary

Well-architected implementation with good test coverage for happy paths. Main concerns are parameter binding (appears broken) and missing error handling.

Overall: ⚠️ Needs Revisions

The code is high quality but has a critical parameter binding issue that must be addressed.

Great work on the architecture! 🎉

@claude
Copy link

claude bot commented Dec 21, 2025

Pull Request Review: SQLite Support

This PR introduces SQLite support to RivetKit with a custom KV-backed VFS implementation.

🎯 Overall Assessment

Quality: High - Well-structured with good separation of concerns
Architecture: Strong - Clean abstraction layers
Risk Level: Medium - Complex VFS implementation

✅ Strengths

  1. Clean architecture: VFS layer isolated from database provider logic
  2. Per-instance isolation: Each SqliteVfs gets its own WASM module
  3. Flexible providers: Both raw SQL and Drizzle ORM support
  4. Schema versioning: BARE schema with embedded versioning
  5. Efficient chunking: 4KB chunks with batch operations
  6. Proper async mutex: Serializes open_v2 calls
  7. Good test coverage: Driver test suite included

🔴 Critical Issues

1. SQL Injection Vulnerabilities (HIGH SEVERITY) ⚠️

Location: rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/actor-db-raw.ts

Multiple actions use string interpolation instead of parameterized queries. Lines 18-20, 47-49, 58, 64-65, 73-75 are vulnerable.

Fix: Use parameterized queries as shown in examples/sqlite-raw/src/registry.ts

⚠️ Major Concerns

2. Memory Management - Native Database Cleanup

drivers/file-system/actor.ts:37 - Native SQLite databases are cached but never closed when actors are destroyed. This causes resource leaks.

3. Inconsistent Parameter Binding

The execute() method accepts variadic args but doesn't use them. The args are captured but never passed to wa-sqlite.

4. Query Return Type Ambiguity

execute() returns unknown[] but actually returns Record<string, unknown>[]. Update types for better type safety.

💡 Suggestions

  1. Make CHUNK_SIZE configurable
  2. Add tests for concurrent operations, large files, edge cases
  3. Document VFS limitations and migration guide
  4. List better-sqlite3 as optional peer dependency
  5. Run and share benchmark results
  6. Improve error handling consistency

🎯 Priority Action Items

Must Fix:

  1. Fix SQL injection vulnerabilities (Critical)
  2. Add database cleanup logic
  3. Clarify parameter binding behavior

Should Fix:
4. Document migration path
5. Add comprehensive tests
6. Run benchmarks

🏁 Conclusion

Solid implementation with clever VFS design. SQL injection vulnerabilities must be fixed before merging.

Recommendation: Request changes for critical security issues.


Generated by Claude Code

@NathanFlurry NathanFlurry changed the base branch from main to graphite-base/3666 February 4, 2026 20:15
@NathanFlurry NathanFlurry changed the base branch from graphite-base/3666 to 02-03-feat_use_cases February 4, 2026 20:15
@graphite-app graphite-app bot force-pushed the 02-03-feat_use_cases branch 2 times, most recently from b644aff to 4202678 Compare February 4, 2026 20:40
@NathanFlurry NathanFlurry changed the base branch from 02-03-feat_use_cases to graphite-base/3666 February 5, 2026 08:34
@NathanFlurry NathanFlurry changed the base branch from graphite-base/3666 to 02-03-feat_use_cases February 6, 2026 07:59
@NathanFlurry NathanFlurry marked this pull request as ready for review February 6, 2026 07:59
Comment on lines +67 to +82
execute: async (query, ...args) => {
const results: Record<string, unknown>[] = [];
let columnNames: string[] | null = null;
await db.exec(query, (row: unknown[], columns: string[]) => {
// Capture column names on first row
if (!columnNames) {
columnNames = columns;
}
// Convert array row to object
const rowObj: Record<string, unknown> = {};
for (let i = 0; i < row.length; i++) {
rowObj[columnNames[i]] = row[i];
}
results.push(rowObj);
});
return results;
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameterized queries silently ignored: The execute method accepts ...args parameters but never uses them. The wa-sqlite exec method only takes a query string and callback, so any arguments passed are silently dropped. This could lead to SQL injection vulnerabilities if developers expect parameterized query support.

execute: async (query, ...args) => {
    // wa-sqlite doesn't support parameterized queries via exec()
    // Either document this limitation or use prepare/bind pattern
    if (args.length > 0) {
        throw new Error('Parameterized queries not supported. Use string interpolation carefully to avoid SQL injection.');
    }
    // ... rest of implementation
}

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@graphite-app
Copy link
Contributor

graphite-app bot commented Feb 6, 2026

Merge activity

  • Feb 6, 8:22 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Feb 6, 8:23 AM UTC: CI is running for this pull request on a draft pull request (#4146) due to your merge queue CI optimization settings.
  • Feb 6, 8:24 AM UTC: Merged by the Graphite merge queue via draft PR: #4146.

graphite-app bot pushed a commit that referenced this pull request Feb 6, 2026
@graphite-app graphite-app bot closed this Feb 6, 2026
@graphite-app graphite-app bot deleted the 12-20-feat_sqlite branch February 6, 2026 08:24
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.

1 participant