Migrations, named parameters, security tests #48
Conversation
- Add Transaction Isolation tests to verify connections cannot access each other's transactions - Add Statement Isolation tests to verify prepared statements are scoped to connections - Add Cursor Isolation tests to verify cursors are scoped to connections - Add Savepoint Isolation tests to verify savepoints belong to owning transactions - Add Concurrent Access Safety tests for thread-safe operations - Add Resource Cleanup tests to verify cleanup on disconnect - Add Pool Isolation tests for multiple connections to same database - Add Cross-Connection Data Isolation tests for separate database files All 12 tests pass, verifying proper ownership tracking and security boundaries.
- Add support for 'strict: true' option in create_table() - STRICT tables enforce type checking (INT, INTEGER, BLOB, TEXT, REAL only) - Can be combined with RANDOM ROWID option - Generates 'STRICT' keyword at end of CREATE TABLE statement - Add tests for SQL generation (execution requires libSQL 3.37+) This implements part of el-z8u (STRICT Tables feature).
Implement full support for executing queries with named parameters (map-based arguments) instead of positional parameters (list-based). Supports all three SQLite syntaxes: - :name (colon prefix) - @name (at-sign prefix) - $name (dollar prefix) Changes: - Add normalize_arguments/3 helper to convert map params to positional list - Add map_to_positional_args/3 to introspect statement and reorder params - Add remove_param_prefix/1 helper to clean parameter names - Update handle_execute (both non-transactional and transactional paths) to normalize args - Add comprehensive test suite with 18 tests covering: - All three parameter syntaxes - Basic CRUD operations (INSERT, SELECT, UPDATE, DELETE) - Transactions (commit and rollback) - Prepared statements with parameter introspection - Edge cases (NULL values, extra params, missing params) - Backward compatibility with positional parameters Tests are thorough and include transaction isolation, error handling, and various parameter combinations. All tests clean up their database files after running. Issue: el-nqb
WalkthroughAdds named-parameter execution with introspection and caching, libSQL table options (STRICT, RANDOM ROWID) and generated columns, extensive tests and teardown cleanup, many issue-tracking and documentation/changelog updates, and local tooling/ignore adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Elixir as lib/ecto_libsql/native.ex
participant Introspect as Introspection (prepare/params)
participant NIF as native NIF (SQLite)
Note over Elixir,Introspect: Named-parameter normalization and execution
Client->>Elixir: query(conn_id, sql, args)
Elixir->>Elixir: normalize_arguments(conn_id, sql, args)
alt args is map (named params)
Elixir->>Introspect: prepare(conn_id, sql) for param names
Introspect->>NIF: prepare_statement(sql)
NIF-->>Introspect: stmt_handle + param metadata
Introspect->>Elixir: ordered param names
Elixir->>Elixir: map_to_positional_args(map, ordered_names) (cache names)
Elixir->>NIF: close_statement(stmt_handle)
else args already positional
Elixir-->>Elixir: use args as-is
end
Elixir->>NIF: query_args(conn_id, sql, positional_args)
NIF-->>Elixir: result or error
Elixir-->>Client: result or wrapped introspection error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
AGENTS.md (1)
1454-1547: Remove duplicated “STRICT Tables (Type Enforcement)” sectionThe “STRICT Tables (Type Enforcement)” section appears twice with effectively the same heading and content (first around the migrations section, then again immediately after RANDOM ROWID). This duplication makes the guide longer to scan and triggers markdown duplicate‑heading warnings.
I’d suggest keeping a single STRICT section (probably the first one near the main migrations discussion) and deleting or trimming the second to a short cross‑reference.
Example diff to drop the duplicate block
-#### STRICT Tables (Type Enforcement) - -STRICT tables enforce strict type checking - columns must be one of the allowed SQLite types. This prevents accidental type mismatches and data corruption: -... -# (lines 1606–1698 removed – second copy of STRICT docs) -... -```Also applies to: 1606-1698
lib/ecto/adapters/libsql/connection.ex (1)
384-396: Generated column support is good; consider a bit more validationThe new
generatedhandling incolumn_options/2cleanly maps:
generated: "expr"→GENERATED ALWAYS AS (expr)(virtual), andgenerated: "expr", stored: true→GENERATED ALWAYS AS (expr) STORED,and is reused consistently for CREATE TABLE and ALTER TABLE paths. That’s a solid extension of the existing DDL helpers. (sqlite.org)
Two small robustness improvements you might consider:
- Guard against
generatedanddefaultbeing set on the same column, since SQLite explicitly disallows defaults on generated columns and will error at runtime anyway. (sqlite.org)- Likewise, reject
generated: ...onprimary_key: true, as generated columns cannot participate in a PRIMARY KEY.Raising an
ArgumentErrorduring migration compilation for these combos would give users a clearer, earlier failure than a lower‑level SQLite error.Sketch of a defensive check
- defp column_options(opts, composite_pk) do + defp column_options(opts, composite_pk) do default = column_default(Keyword.get(opts, :default)) null = if Keyword.get(opts, :null) == false, do: " NOT NULL", else: "" @@ - generated = - case Keyword.get(opts, :generated) do + generated_opt = Keyword.get(opts, :generated) + + if generated_opt && Keyword.get(opts, :default) do + raise ArgumentError, + "generated columns cannot have a default value in SQLite" + end + + if generated_opt && Keyword.get(opts, :primary_key) && !composite_pk do + raise ArgumentError, + "generated columns cannot be used as PRIMARY KEY in SQLite" + end + + generated = + case generated_opt do nil -> ""test/security_test.exs (4)
1-3: Consider addingasync: falseexplicitly.The module uses
use ExUnit.Casewithout specifyingasync: trueorasync: false. While the default isasync: falseforExUnit.Case, explicitly setting it improves readability and prevents accidental changes. Given these tests involve database connections and file-based SQLite databases,async: falseis the safer choice.defmodule EctoLibSql.SecurityTest do - use ExUnit.Case + use ExUnit.Case, async: false
163-173: Loop variable shadowing may cause confusion.The loop rebinds
stateon each iteration, but the returnedstateon line 173 comes from the outer scope (line 152), not the loop. While functionally correct (the inserts don't need to propagate state changes), this pattern can confuse readers. Consider using_statein the loop to signal intent:for i <- 1..10 do - {:ok, _, _, state} = + {:ok, _, _, _state} = EctoLibSql.handle_execute( "INSERT INTO test_data (value) VALUES (?)", ["value_#{i}"], [], state ) end
258-263: Cleanup pattern matching could be more defensive.The rollback calls don't pattern match on the result, which could hide errors during cleanup. Consider using pattern matching to ensure cleanup succeeds:
# Cleanup - EctoLibSql.handle_rollback([], state_a) - EctoLibSql.handle_rollback([], state_b) + _ = EctoLibSql.handle_rollback([], state_a) + _ = EctoLibSql.handle_rollback([], state_b) EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b)Alternatively, wrap in a try/after block to ensure disconnect always runs.
400-409: Unused variable and incomplete verification.The
stmt_idvariable is prepared but never used. The test also doesn't verify that resources are actually inaccessible after disconnect. Consider either:
- Using the prepared statement before disconnect, then verifying it fails after
- Removing the unused
stmt_idif the intent is just to verify disconnect doesn't crash- {:ok, stmt_id} = EctoLibSql.Native.prepare(state, "SELECT * FROM cleanup_test") + {:ok, _stmt_id} = EctoLibSql.Native.prepare(state, "SELECT * FROM cleanup_test") # Close connection :ok = EctoLibSql.disconnect([], state) - # Resources should not be accessible (they belong to a closed connection) - # This is more of a manual verification - in production would need monitoring - # For now, just verify that closing doesn't crash + # Verify that closing doesn't crash - resource cleanup is handled internally assert truetest/named_parameters_execution_test.exs (1)
460-480: Case-sensitivity test could be more explicit.The test comment says "Parameter names should be case-sensitive" but doesn't actually test case sensitivity by using mixed-case parameter names. Consider adding a test that explicitly verifies case matters:
test "Named parameters are case-sensitive", %{state: state} do # Insert with lowercase parameter names {:ok, _, _, state} = EctoLibSql.handle_execute( "INSERT INTO users (id, name, email, age) VALUES (:id, :name, :email, :age)", %{id: 1, name: "Test", email: "test@example.com", age: 25}, [], state ) # Query with uppercase key should fail (assuming case-sensitivity) result = EctoLibSql.handle_execute( "SELECT * FROM users WHERE id = :id", %{ID: 1}, # Uppercase key [], state ) assert match?({:error, _, _}, result) end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.beads/issues.jsonl.claude/settings.local.jsonAGENTS.mdCHANGELOG.mdlib/ecto/adapters/libsql/connection.exlib/ecto_libsql/native.extest/ecto_migration_test.exstest/named_parameters_execution_test.exstest/security_test.exs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto_libsql/native.exlib/ecto/adapters/libsql/connection.ex
AGENTS.md
📄 CodeRabbit inference engine (AGENT.md)
Document agent architecture and design patterns in AGENTS.md
Files:
AGENTS.md
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exstest/ecto_migration_test.exs.beads/issues.jsonllib/ecto_libsql/native.exlib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exstest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exslib/ecto_libsql/native.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
CHANGELOG.mdtest/ecto_migration_test.exsAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exslib/ecto_libsql/native.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
CHANGELOG.mdtest/security_test.exstest/named_parameters_execution_test.exs.beads/issues.jsonlAGENTS.md
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/security_test.exstest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
test/ecto_migration_test.exslib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
test/ecto_migration_test.exslib/ecto/adapters/libsql/connection.exAGENTS.md
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Test coverage must include: happy path, error cases, edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
.beads/issues.jsonl
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Elixir modules must follow the structure: EctoLibSql (DBConnection protocol), EctoLibSql.Native (NIF wrappers), EctoLibSql.State (connection state), Ecto.Adapters.LibSql (main adapter), Ecto.Adapters.LibSql.Connection (SQL generation)
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-05T22:12:22.789Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENT.md:0-0
Timestamp: 2025-12-05T22:12:22.789Z
Learning: Maintain agent implementation documentation standards
Applied to files:
AGENTS.md
📚 Learning: 2025-12-05T22:12:22.789Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENT.md:0-0
Timestamp: 2025-12-05T22:12:22.789Z
Learning: Applies to AGENTS.md : Document agent architecture and design patterns in AGENTS.md
Applied to files:
AGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
AGENTS.md
🧬 Code graph analysis (4)
test/security_test.exs (1)
lib/ecto_libsql.ex (8)
connect(77-108)handle_execute(148-160)handle_begin(169-174)handle_commit(177-179)handle_commit(188-198)disconnect(131-133)handle_declare(323-341)handle_fetch(272-299)
test/named_parameters_execution_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-160)lib/ecto_libsql/native.ex (5)
begin(509-519)commit(534-538)rollback(551-555)prepare(632-640)close_stmt(716-718)
test/ecto_migration_test.exs (2)
lib/ecto/adapters/libsql/connection.ex (10)
execute_ddl(200-203)execute_ddl(205-208)execute_ddl(210-233)execute_ddl(235-243)execute_ddl(245-253)execute_ddl(255-258)execute_ddl(260-263)execute_ddl(265-268)execute_ddl(270-276)query(690-692)lib/ecto_libsql/native.ex (1)
query(373-422)
lib/ecto_libsql/native.ex (3)
native/ecto_libsql/src/statement.rs (3)
prepare_statement(30-71)statement_parameter_count(313-338)statement_parameter_name(361-391)native/ecto_libsql/src/query.rs (1)
query_args(33-116)native/ecto_libsql/src/transaction.rs (2)
query_with_trx_args(322-401)execute_with_transaction(283-307)
🪛 GitHub Actions: CI
test/named_parameters_execution_test.exs
[error] 419-428: mix format --check-formatted failed. The following lines are not formatted as per formatting rules: lines 419-428 in test/named_parameters_execution_test.exs. Run 'mix format' to fix formatting.
🪛 LanguageTool
AGENTS.md
[style] ~1492-~1492: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...oint values only - NULL - NULL values only (rarely used) Usage Examples: ```...
(ADVERB_REPETITION_PREMIUM)
[style] ~1644-~1644: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...oint values only - NULL - NULL values only (rarely used) Usage Examples: ```...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
1606-1606: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ 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). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
.claude/settings.local.json (1)
46-47: Newgit checkoutpermission meaningfully increases agent write powerAllowing
Bash(git checkout:*)lets agents mutate the working tree (switch branches, reset paths, discard local changes). That’s probably fine for this repo, but it’s a bigger hammer than the existing read‑only git commands.Please just double‑check this broader capability is intentional in your threat model for agent commands.
CHANGELOG.md (1)
12-61: Named-parameters changelog entry is clear and well-scopedThe new “Named Parameters Execution Support” section accurately documents the three SQLite syntaxes, map-based binding, execution paths, and test coverage, and the examples match the public APIs used elsewhere in the docs.
Looks good as a release note for this feature set.
.beads/issues.jsonl (1)
1-32: Backlog metadata updates look consistent with implemented featuresThe enriched issue entries for extensions, RANDOM ROWID, ALTER COLUMN, cross-connection security tests, max_write_replication_index, named parameters, and STRICT tables are structurally valid JSONL and line up with the code and docs added in this PR. The embedded acceptance criteria will be useful for future traceability.
No changes needed here.
lib/ecto/adapters/libsql/connection.ex (1)
443-459: Suffix list approach for RANDOM ROWID and STRICT looks correctThe refactor to accumulate table suffixes in a
suffixeslist and thenEnum.join/1is a nice improvement over a single hard-coded suffix: it naturally supports combinations likeRANDOM ROWIDandSTRICTin either order and keeps the RANDOM ROWID validation logic unchanged.As long as your migration tests cover:
options: [random_rowid: true],options: [strict: true], andoptions: [random_rowid: true, strict: true],this should be safe and future‑proof if additional suffix options are added later.
test/ecto_migration_test.exs (2)
761-806: LGTM! Good coverage of libSQL-specific table options.The tests appropriately validate both SQL generation and actual table creation for
RANDOM ROWID. The STRICT test sensibly only checks SQL generation since older libSQL versions may not support it. Based on learnings, usingRANDOM ROWIDfor security-sensitive tables like sessions helps prevent ID enumeration attacks.
808-844: Good SQL generation tests for generated columns.The tests properly verify the DDL syntax for both virtual and stored generated columns. Consider adding execution tests in a follow-up to verify generated columns work correctly at runtime (insert rows, verify computed values).
test/named_parameters_execution_test.exs (1)
312-376: Good transaction coverage with named parameters.The tests properly verify that named parameters work within transactions and that commit/rollback semantics are preserved. The use of
initial_statevsstatecorrectly handles the state transitions during transaction lifecycle.lib/ecto_libsql/native.ex (3)
303-311: Clean implementation of prefix removal.The function correctly handles all three named parameter prefixes (
:,@,$) documented in the SQL syntax. TheString.slice(name, 1..-1//1)syntax is clear and idiomatic.
286-301: Good normalisation logic with sensible defaults.The function correctly handles the three expected cases: lists (positional), maps (named), and pass-through for anything else. The catch-all ensures forward compatibility but will let invalid types through to the execution layer where they'll produce an appropriate error.
378-381: Good integration of named parameter normalisation.The normalisation is consistently applied in both
query/3andexecute_with_trx/3paths, ensuring named parameters work identically in and out of transactions. The existing result handling is preserved.Also applies to: 430-431, 445-445, 474-474
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
lib/ecto/adapters/libsql/connection.ex (1)
374-409: Generated-column validation and table suffixes look solid; double-check GENERATED clause orderingThe new checks in
column_options/2that reject:generatedcombined with:defaultor:primary_keycorrectly enforce SQLite’s constraints on generated columns, and thegeneratedfragment itself is composed cleanly with optionalSTORED. The revisedtable_options/2logic that accumulates suffixes intosuffixes(allowingRANDOM ROWIDandSTRICTto co-exist) is also straightforward and reads well.One thing worth verifying against SQLite/libSQL’s exact DDL grammar is the ordering of
GENERATED ALWAYS AS (...)relative toNOT NULLand other column constraints, since you currently emitpk/null/defaultbeforegenerated. If SQLite expects the generated clause before constraints for computed columns, you may need to reorder those fragments to avoid DDL parse errors on real migrations.Also applies to: 418-475
test/named_parameters_execution_test.exs (2)
16-43: Consider explicitly disconnecting in setup teardownThe
on_exit/1block removes the temp DB and its WAL/SHM files, which is great, but it never callsEctoLibSql.disconnect/2. Explicitly disconnecting the connection here would keep test resource handling consistent with other modules (likePragmaTest) and avoid leaving open NIF resources until the VM shuts the test process down.
404-503: Edge-case coverage for named parameters is strongThe tests for missing parameters, extra ignored keys,
NULLvalues, and case sensitivity give good confidence around the new normalisation layer and error paths. Once you adjust the underlying implementation to fully support both atom and string keys (see comments onnormalize_arguments/3), these cases will provide a solid regression suite; consider adding one or two tests that pass string-key maps as well, since AGENTS.md documents that shape.test/ecto_migration_test.exs (1)
762-873: Good coverage of libSQL table options and generated columns; consider an execution-level checkThe new tests around
RANDOM ROWID,STRICT, and virtual/stored generated columns line up well with the DDL changes inConnection.execute_ddl/1and give nice regression coverage on the emitted SQL. At the moment the generated-column examples only assert on the SQL string without actually running the DDL; once you’re confident in the syntax ordering, adding a migration/execution test (e.g. creating the table and inserting/selecting across a generated column) would catch any subtle syntax incompatibilities with libSQL/SQLite that pure string assertions can’t.AGENTS.md (1)
1456-1548: STRICT table documentation is clear; tweak stmt_parameter_name notes for consistencyThe STRICT tables section gives a clear, practical overview of how to enable
strict: true, what types are allowed, and what the resulting SQL looks like; the combined example withrandom_rowid: truematches the new DDL behaviour in the adapter.One small consistency issue: in the earlier
stmt_parameter_name/3docs you still describe stripping the:,@, or$prefix and returning"id","name", etc, whereas the new tests now expect the raw parameter token (e.g.":id"). It’d be good to update that paragraph and the example values to match whichever behaviour you’ve standardised on in the NIF/Elixir wrapper so consumers don’t get surprised by the returned name format.test/security_test.exs (2)
4-9: Consider explicit error handling in cleanup helper.The
File.rm/1calls ignore all errors, including permission issues or filesystem errors. While this is often acceptable for test cleanup (we want cleanup to proceed even if files don't exist), it may silently mask real issues.🔎 Proposed refactor with explicit error handling
defp cleanup_db(db_path) do - File.rm(db_path) - File.rm(db_path <> "-wal") - File.rm(db_path <> "-shm") + Enum.each([db_path, db_path <> "-wal", db_path <> "-shm"], fn path -> + case File.rm(path) do + :ok -> :ok + {:error, :enoent} -> :ok + {:error, reason} -> IO.warn("Failed to cleanup #{path}: #{inspect(reason)}") + end + end) end
1-2: Consider usingasync: truefor independent test isolation.The suite sets
async: falseat line 2, which serializes all tests. While some tests check concurrent behaviour and isolation, most describe blocks use unique database files per test (viaSystem.unique_integer()), suggesting tests could run in parallel without interference.Consider:
- Using
async: trueto speed up test execution- If specific tests require serialization, use
@tag :serialon those tests only- Ensure proper cleanup in all tests to prevent cross-test pollution
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AGENTS.mdlib/ecto/adapters/libsql/connection.exlib/ecto_libsql/native.extest/ecto_migration_test.exstest/error_handling_test.exstest/named_parameters_execution_test.exstest/pragma_test.exstest/security_test.exs
🧰 Additional context used
📓 Path-based instructions (2)
AGENTS.md
📄 CodeRabbit inference engine (AGENT.md)
Document agent architecture and design patterns in AGENTS.md
Files:
AGENTS.md
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto/adapters/libsql/connection.exlib/ecto_libsql/native.ex
🧠 Learnings (21)
📚 Learning: 2025-12-05T22:12:22.789Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENT.md:0-0
Timestamp: 2025-12-05T22:12:22.789Z
Learning: Maintain agent implementation documentation standards
Applied to files:
AGENTS.md
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.extest/pragma_test.exstest/error_handling_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Elixir modules must follow the structure: EctoLibSql (DBConnection protocol), EctoLibSql.Native (NIF wrappers), EctoLibSql.State (connection state), Ecto.Adapters.LibSql (main adapter), Ecto.Adapters.LibSql.Connection (SQL generation)
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exslib/ecto/adapters/libsql/connection.extest/pragma_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
AGENTS.mdlib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.extest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
AGENTS.mdtest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
AGENTS.mdtest/ecto_migration_test.exslib/ecto/adapters/libsql/connection.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
AGENTS.mdtest/named_parameters_execution_test.exstest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
AGENTS.mdtest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/named_parameters_execution_test.exstest/error_handling_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto/adapters/libsql/connection.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Applied to files:
test/pragma_test.exstest/error_handling_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/pragma_test.exstest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/pragma_test.exstest/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/error_handling_test.exstest/security_test.exslib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/error_handling_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Run Elixir tests with `mix test`, Rust tests with `cd native/ecto_libsql && cargo test`, and use `--trace` flag for debugging specific tests
Applied to files:
test/error_handling_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
🧬 Code graph analysis (3)
test/named_parameters_execution_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-160)lib/ecto_libsql/native.ex (5)
begin(566-576)commit(591-595)rollback(608-612)prepare(689-697)close_stmt(773-775)
test/ecto_migration_test.exs (1)
lib/ecto/adapters/libsql/connection.ex (9)
execute_ddl(200-203)execute_ddl(205-208)execute_ddl(210-233)execute_ddl(235-243)execute_ddl(245-253)execute_ddl(255-258)execute_ddl(260-263)execute_ddl(265-268)execute_ddl(270-276)
test/security_test.exs (2)
lib/ecto_libsql.ex (9)
connect(77-108)handle_execute(148-160)handle_begin(169-174)handle_commit(177-179)handle_commit(188-198)disconnect(131-133)handle_declare(323-341)handle_fetch(272-299)handle_rollback(207-215)lib/ecto_libsql/native.ex (5)
prepare(689-697)query_stmt(741-760)close_stmt(773-775)create_savepoint(1498-1500)rollback_to_savepoint_by_name(1570-1572)
🪛 LanguageTool
AGENTS.md
[style] ~1494-~1494: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...oint values only - NULL - NULL values only (rarely used) Usage Examples: ```...
(ADVERB_REPETITION_PREMIUM)
⏰ 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: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
test/pragma_test.exs (1)
258-280: Nice WAL/SHM cleanup for the second connectionExtending teardown to remove
-waland-shmfortest_db2keeps the test sandbox tidy and avoids leaking ancillary SQLite files between runs. Looks good.test/error_handling_test.exs (1)
185-226: Consistent cleanup of WAL/SHM artefactsAdding
File.rm(test_db <> "-wal")andFile.rm(test_db <> "-shm")in the concurrent test’s teardown aligns this test with others and avoids stray SQLite files, even if/when the tag is removed and the test is enabled.test/named_parameters_execution_test.exs (1)
380-401: The documentation in AGENTS.md already correctly specifies thatstmt_parameter_name/3returns names WITH their prefixes (:name,@name,$name), which aligns with the test expectations. The inline comments inlib/ecto_libsql/native.ex(showing unprefixed names like"id") are the outdated documentation that should be corrected to match the actual implementation and public API documentation.Likely an incorrect or invalid review comment.
AGENTS.md (1)
488-597: Update documentation to match actual parameter handling behaviourThe "Named Parameters" section shows examples that don't align with the current implementation:
Examples use string-keyed maps (
%{"email" => ...}) buthandle_execute/4expects atom-keyed maps (%{email: ...}) after the normalisation layer strips parameter prefixes and converts keys to atoms. String-keyed maps will silently receivenilvalues.Examples pass maps to prepared statement functions like
query_stmt(state, stmt_id, %{"email" => ...})but these functions don't support map arguments. They expect lists and pass arguments directly to NIFs that don't include the normalisation layer. The documentation should clarify that prepared statements currently accept lists only.Update the examples to either:
- Use atom-keyed maps for
handle_execute/4and lists forquery_stmt/3andexecute_stmt/4, or- Extend prepared statement helpers to also support map-based named parameters with the same normalisation logic, and confirm that both atom- and string-keyed maps are supported as advertised.
test/security_test.exs (2)
328-359: No issues identified. The test correctly validates that concurrent cursor fetches are safe due to mutex-protected access in the Rust implementation, and the manual disconnect call does not interfere with ExUnit'son_exitcleanup callback.
456-460: Remove this comment—the test assertion is correct.The cursor properly returns
{[], [], 0}after disconnect because cursors are not automatically cleaned up when a connection closes. Unlike prepared statements (which validate the connection still exists),fetch_cursoronly verifies cursor ownership, not connection validity. The tuple{[], [], 0}correctly indicates the cursor is exhausted, not that it was cleaned up. Cursors must be explicitly closed via a separate operation.
- Fix struct update pattern in security_test.exs (use map update after pattern match assertion) - Replace unreachable :deallocated clause with :halt in cursor test - Fix unused variable warnings (_state and _i) - Improve test cleanup to properly stop repo before file removal - Add -journal file cleanup to prevent stale files
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
test/security_test.exs (5)
44-59: Overly lenient assertion weakens test validation.This case statement still accepts both error and success outcomes, which defeats the purpose of the isolation test. The test should definitively expect one outcome.
🔎 Recommended fix
case EctoLibSql.handle_execute( "SELECT 1", [], [], state_b_fake ) do {:error, _reason, _state} -> # Expected - transaction belongs to connection A assert true {:ok, _, _, _} -> - # If execution succeeds, the system should prevent the transaction - # from being used across connections anyway. The key is no crash. - # SQLite will likely error on the transaction ID being invalid - assert true + flunk("Connection B should not be able to use connection A's transaction") end
147-149: Manual disconnect of setup-provided state breaks subsequent tests.Line 147 disconnects
state_awhich is provided by the setup block (line 120 shows it comes from setup context). This will cause issues if other tests in this describe block attempt to use the shared connection. Only disconnect resources created within the test (state_b).🔎 Recommended fix
# Cleanup EctoLibSql.Native.close_stmt(stmt_id_a) - EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b) cleanup_db(db_path_b)
166-168: Manual disconnect of setup-provided state breaks subsequent tests.Line 167 disconnects
statewhich comes from the setup block. Remove the manual disconnect and let the test framework handle cleanup.🔎 Recommended fix
{:ok, _} -> flunk("Should not be able to use a closed statement") end - - EctoLibSql.disconnect([], state) end
242-244: Manual disconnect of setup-provided state breaks subsequent tests.Line 242 disconnects
state_awhich is provided by the setup block. Only disconnect resources created within the test (state_b).🔎 Recommended fix
- EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b) cleanup_db(db_path_b)
412-414: Manual disconnect of setup-provided state breaks subsequent tests.Line 412 disconnects
state_awhich is provided by the setup block. Only disconnect resources created within the test (state_b).🔎 Recommended fix
# Cleanup EctoLibSql.handle_commit([], state_a) EctoLibSql.handle_commit([], state_b) - EctoLibSql.disconnect([], state_a) EctoLibSql.disconnect([], state_b) cleanup_db(db_path_b)
🧹 Nitpick comments (5)
test/security_test.exs (3)
185-193: State variable not threaded through loop iterations.The
forcomprehension captures the result in_state(discarded), but continues using the originalstatevariable. While this may work for SQLite due to autocommit semantics, it's inconsistent with the pattern used elsewhere in the codebase where state is explicitly updated.🔎 Suggested improvement
- for i <- 1..10 do - {:ok, _, _, _state} = - EctoLibSql.handle_execute( - "INSERT INTO test_data (value) VALUES (?)", - ["value_#{i}"], - [], - state - ) - end + Enum.each(1..10, fn i -> + {:ok, _, _, _} = + EctoLibSql.handle_execute( + "INSERT INTO test_data (value) VALUES (?)", + ["value_#{i}"], + [], + state + ) + end)Using
Enum.each/2makes it explicit that the result isn't being accumulated.
249-249: Empty map pattern in test context is unusual.The test signature uses
%{}which doesn't extract any values from the setup context. If this test doesn't require setup context, consider removing the pattern match entirely, or verify the test belongs in a describe block without shared setup.- test "savepoint belongs to owning transaction", %{} do + test "savepoint belongs to owning transaction" do
312-320: State variable not threaded through loop iterations.Same issue as line 185-193 — consider using
Enum.each/2to make the intent explicit.test/ecto_migration_test.exs (2)
818-834: Consider executing generated column SQL to validate runtime behaviour.The test only verifies SQL generation. While the
!String.contains?(sql, "STORED")assertion is correct, usingrefute sql =~ "STORED"would be more idiomatic in ExUnit.🔎 Suggested improvement
# Verify GENERATED clause appears in SQL (but not STORED) assert sql =~ "GENERATED ALWAYS AS" assert sql =~ "first_name || ' ' || last_name" - assert !String.contains?(sql, "STORED") + refute sql =~ "STORED"
799-814: Consider documenting minimum libSQL version requirement for STRICT support.SQLite 3.37.0+ (and thus libSQL builds based on that version) supports STRICT tables. The test verifies SQL generation only—adding a comment specifying the minimum required libSQL version or Ecto version would help future maintainers understand the compatibility requirement. This is optional since the project already uses libSQL 0.9.29 which fully supports STRICT.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/ecto_migration_test.exstest/fuzz_test.exstest/security_test.exs
✅ Files skipped from review due to trivial changes (1)
- test/fuzz_test.exs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
test/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/security_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Run Elixir tests with `mix test`, Rust tests with `cd native/ecto_libsql && cargo test`, and use `--trace` flag for debugging specific tests
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Applied to files:
test/ecto_migration_test.exs
🧬 Code graph analysis (2)
test/security_test.exs (2)
lib/ecto_libsql.ex (9)
connect(77-108)handle_execute(148-160)handle_begin(169-174)handle_commit(177-179)handle_commit(188-198)disconnect(131-133)handle_declare(323-341)handle_fetch(272-299)handle_rollback(207-215)lib/ecto_libsql/native.ex (5)
prepare(689-697)query_stmt(741-760)close_stmt(773-775)create_savepoint(1498-1500)rollback_to_savepoint_by_name(1570-1572)
test/ecto_migration_test.exs (1)
lib/ecto/adapters/libsql/connection.ex (10)
execute_ddl(200-203)execute_ddl(205-208)execute_ddl(210-233)execute_ddl(235-243)execute_ddl(245-253)execute_ddl(255-258)execute_ddl(260-263)execute_ddl(265-268)execute_ddl(270-276)query(703-705)
⏰ 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: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
test/security_test.exs (4)
4-9: LGTM!Clean helper function for removing SQLite database files and associated artifacts (WAL, SHM).
494-564: Good test coverage for pool-like isolation.This test validates that separate connections to the same database file maintain independent transaction contexts. The approach of committing conn1 first before conn2's insert correctly handles SQLite's write serialisation.
567-627: Good isolation test for separate database files.Validates that connections to different database files are completely isolated at the schema level.
456-461: Test expectation does not match implementation behaviour.The test expects
{[], [], 0}after disconnect, but the Rustfetch_cursorimplementation (cursor.rs:297–301) will return(columns, [], 0)— it includes the column names even when no rows remain. Cursors are not removed from the registry when a connection closes, so the cursor ownership check succeeds and returns the stored columns rather than an empty array.The inconsistency with statement behaviour (which returns
{:error, _}after disconnect) remains valid: statements are designed to fail after connection loss, whilst cursors return the column metadata with zero rows. However, the test assertion should be updated to match the actual implementation:{columns, [], 0}wherecolumnsis not empty but contains the result column names.test/ecto_migration_test.exs (4)
14-37: Good improvements to test setup/teardown.The enhanced teardown properly:
- Checks process liveness before stopping
- Adds a delay to ensure file handles are released
- Cleans up journal file in addition to WAL/SHM
This aligns with the broader cleanup enhancements seen in other test files.
771-797: Good test coverage for RANDOM ROWID libSQL extension.The test correctly verifies both SQL generation and actual execution, then confirms the table schema in
sqlite_master. Based on learnings, RANDOM ROWID is recommended for security-sensitive tables to prevent ID enumeration attacks.
836-852: LGTM!Good test for stored generated columns. The assertions correctly verify the presence of
GENERATED ALWAYS AS,STORED, and the expression.
854-881: Good constraint validation tests.These tests properly verify that the adapter rejects invalid column configurations at DDL generation time:
- Generated columns with DEFAULT values
- Generated columns as primary keys
Both are SQLite constraints that are correctly enforced early.
Addresses CodeRabbit review comment: query_stmt and execute_stmt now properly normalise map arguments to positional lists using stmt introspection. - Add normalise_arguments_for_stmt/3 for prepared statement parameter conversion - Update execute_stmt/4 to normalise args before execution - Update query_stmt/3 to normalise args before query - Add comprehensive tests for named parameters with prepared statements - Update documentation to reflect both positional and named parameter support - Use British English spelling (normalise vs normalize) per project convention
Change the test to assert that using connection A's transaction on connection B fails with an error, rather than accepting both success and error outcomes. This ensures the security test actually validates that cross-connection transaction usage is rejected.
…test The test was disconnecting state_a which is the shared connection from setup, potentially causing issues. Now only per-test resources (state_b) are managed in the test, leaving the shared state to setup/teardown.
Security tests: - Remove manual disconnects of shared setup-provided state - Make cross-connection isolation test strict (assert error, not accept both) - Tests now properly manage only per-test resources Credo fixes: - Replace String.to_atom/1 with String.to_existing_atom/1 to avoid creating atoms at runtime (security best practice) - Add get_map_value_flexible/2 helper to support both atom and string keys in parameter maps - Parameter names now stored as strings internally, with flexible lookup
Fixes compiler warning that was causing CI to fail with --warnings-as-errors.
Use Enum.reduce instead of for comprehension to properly thread the state variable through each INSERT operation, consistent with patterns used elsewhere in the codebase.
- Remove unnecessary %{} pattern match from savepoint test that has no
shared setup context
- Fix state threading in concurrent access setup using Enum.reduce
Replace assert !String.contains?(sql, "STORED") with the more idiomatic ExUnit refute sql =~ "STORED" pattern.
Apply consistent anonymous `_` pattern for unused variables across test
files to satisfy Credo's consistency checks:
- security_test.exs: Simplify patterns like `{:error, _reason, _state}`
to `{:error, _, _}`, remove unused `db_path` from test context params
- fuzz_test.exs: Replace named unused vars (_state, _reason, _query, _e,
_count, _result, _final_state) with anonymous `_`, fix number format
(10000 → 10_000)
- named_parameters_execution_test.exs: Standardise `_state` → `_`
All 487 tests pass, Credo reports no consistency issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/security_test.exs (1)
4-9: Tightened security tests look good; consider also cleaning-journalfiles.The new isolation tests (transactions, statements, cursors, savepoints, pools, cross‑DB) now assert failures explicitly and avoid double‑disconnecting shared state, which is a solid improvement for security validation. The only minor thing:
cleanup_db/1currently removes the main file,-wal, and-shmbut not-journal; adding that would align with other tests and avoid stray journal files on non‑WAL setups.Also applies to: 567-627
lib/ecto_libsql/native.ex (2)
286-414: Named-parameter normalisation and caching are well-structured; watch unboundedpersistent_termgrowth.The
normalise_arguments/3/normalise_arguments_for_stmt/3pipeline plus parameter‑name caching gives a clean story for converting map args (including mixed:name,@name,$namesyntaxes) into positional lists, and the flexible atom/string key lookup avoids runtime atom creation. The only thing to keep in mind long‑term is that@param_cache_keystores a global map in:persistent_termkeyed by raw SQL; in workloads that generate many distinct statements, this could grow without bound. If you later see very dynamic SQL usage, consider capping this cache or switching to an ETS‑backed structure with eviction, but it’s fine as‑is for typical Ecto workloads.Also applies to: 420-459
366-380: Consider propagating parameter-introspection failures instead of treating them as zero-parameter statements.In
introspect_and_cache_params/3, any non‑integer result fromstatement_parameter_count/2falls back to0, which then yields an emptyparam_nameslist and an empty positional args list. Ifstatement_parameter_count/2ever returns{:error, reason}(e.g. invalidstmt_id), the calling code will silently proceed with no parameters rather than surfacing the error.Given you already propagate preparation failures as
{:error, reason}andquery/3/execute_with_trx/3turn{:error, reason}fromnormalise_arguments/3into a clearEctoLibSql.Error, it would be more consistent to treat{:error, reason}fromstatement_parameter_count/2similarly instead of coercing it to0. That keeps mis‑bound or invalid statements from being hidden behind confusing SQL errors.Also applies to: 410-414, 472-483, 539-548
test/named_parameters_execution_test.exs (1)
16-43: Comprehensive named-parameter tests; optionally align DB cleanup with other suites.The coverage here is excellent: all three named syntaxes, positional fallback, transactions, prepared statements, and edge cases (missing/extra params, NULL, case sensitivity) map cleanly onto the new normalisation behaviour in
EctoLibSql.Native. One small consistency tweak you might consider is extending theon_exitcleanup to removedb_name <> "-journal"as well, mirroring other tests, so SQLite journal files don’t linger on non‑WAL configurations.Also applies to: 380-577
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.beads/issues.jsonl.claude/settings.local.jsonlib/ecto_libsql/native.extest/ecto_migration_test.exstest/fuzz_test.exstest/named_parameters_execution_test.exstest/security_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/settings.local.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ex
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ex: Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Use cursors and DBConnection.stream for memory-efficient processing of large result sets instead of loading all rows into memory
Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Use Ecto's insert_all/3 for bulk inserts of pre-mapped data to leverage batching and achieve better performance than individual inserts
Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Use vector similarity search with EctoLibSql.Native.vector_distance_cos/2 for semantic search features in AI/ML applications
Encrypt local databases and replicas using encryption_key parameter (minimum 32 characters) for protecting sensitive data at rest
Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Files:
lib/ecto_libsql/native.ex
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: When adding SQLite function support, update `lib/ecto/adapters/libsql/connection.ex` with the expression handler and add corresponding tests
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exs.beads/issues.jsonltest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Elixir modules must follow the structure: EctoLibSql (DBConnection protocol), EctoLibSql.Native (NIF wrappers), EctoLibSql.State (connection state), Ecto.Adapters.LibSql (main adapter), Ecto.Adapters.LibSql.Connection (SQL generation)
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Configure WAL (Write-Ahead Logging) mode using EctoLibSql.Pragma.set_journal_mode/2 for better concurrency and write performance in local databases
Applied to files:
test/fuzz_test.exstest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Always pattern match on result tuples from EctoLibSql operations to handle {:ok, ...} and {:error, ...} cases, avoiding bare pattern matches that could crash the process
Applied to files:
test/fuzz_test.exstest/named_parameters_execution_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, always use `ok_or_else()` with proper error messages for handling Option types instead of `.unwrap()`
Applied to files:
test/fuzz_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : NEVER use `.unwrap()` in production Rust code - use `safe_lock` helpers instead. Tests MAY use `.unwrap()` for simplicity
Applied to files:
test/fuzz_test.exstest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Use proper transactions with timeout in Elixir: `Repo.transaction(fn -> ... end, timeout: 15_000)` to handle database locked errors
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use savepoints (EctoLibSql.Native.create_savepoint/2) within transactions for granular error recovery on partial batch failures
Applied to files:
test/fuzz_test.exstest/named_parameters_execution_test.exs.beads/issues.jsonltest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use transaction behaviours (deferred, immediate, exclusive, read_only) with EctoLibSql.Native.begin/2 to optimize for specific workload patterns and prevent writer starvation
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust async code, drop locks before calling `TOKIO_RUNTIME.block_on()` to prevent deadlocks
Applied to files:
test/fuzz_test.exstest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock_arc(&client, "function_name client")` for locking Arc<Mutex<T>> instead of `.lock().unwrap()`
Applied to files:
test/fuzz_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use Ecto changesets with proper validation and error handling instead of raw SQL for INSERT/UPDATE operations to leverage Ecto's safety features
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exstest/ecto_migration_test.exstest/security_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use prepared statements with automatic statement caching for repeated queries in EctoLibSql to improve performance (~10-15x faster than unprepared queries)
Applied to files:
test/fuzz_test.exslib/ecto_libsql/native.extest/named_parameters_execution_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: For type conversion issues, update loaders/dumpers in `lib/ecto/adapters/libsql.ex` following the pattern `loaders/dumpers` functions
Applied to files:
lib/ecto_libsql/native.ex.beads/issues.jsonltest/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use statement introspection functions (stmt_parameter_count, stmt_column_count, stmt_column_name) to validate prepared statement structure before execution
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Use batch operations (batch/2, batch_transactional/2, or execute_batch_sql/2) instead of individual INSERT/UPDATE/DELETE statements for bulk data operations
Applied to files:
lib/ecto_libsql/native.ex
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Test coverage must include: happy path, error cases, edge cases (NULL values, empty strings, large datasets), transaction rollback scenarios, type conversions, and concurrent operations
Applied to files:
.beads/issues.jsonl
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Use RANDOM ROWID in table definitions (with options: [random_rowid: true]) for security-sensitive tables to prevent ID enumeration attacks
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to priv/repo/migrations/*.exs : Create appropriate indexes (simple, composite, covering) on frequently queried columns to optimize query performance
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.161Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.161Z
Learning: Run Elixir tests with `mix test`, Rust tests with `cd native/ecto_libsql && cargo test`, and use `--trace` flag for debugging specific tests
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:32.937Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-31T08:05:32.937Z
Learning: Applies to **/*.ex : Enable foreign key constraints in SQLite using EctoLibSql.Pragma.enable_foreign_keys/1 for data integrity in production databases
Applied to files:
test/ecto_migration_test.exs
📚 Learning: 2025-12-31T08:05:13.160Z
Learnt from: CR
Repo: ocean/ecto_libsql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-31T08:05:13.160Z
Learning: Applies to native/ecto_libsql/src/!(tests)/**/*.rs : In Rust, use `safe_lock(®ISTRY, "function_name context")` for locking registries instead of `.lock().unwrap()`
Applied to files:
test/ecto_migration_test.exstest/security_test.exs
🧬 Code graph analysis (3)
test/fuzz_test.exs (1)
lib/ecto_libsql.ex (2)
handle_rollback(207-215)handle_execute(148-160)
lib/ecto_libsql/native.ex (3)
native/ecto_libsql/src/statement.rs (5)
prepare_statement(30-71)statement_parameter_count(313-338)statement_parameter_name(361-391)execute_prepared(164-221)query_prepared(86-145)native/ecto_libsql/src/query.rs (1)
query_args(33-116)native/ecto_libsql/src/transaction.rs (1)
query_with_trx_args(322-401)
test/named_parameters_execution_test.exs (2)
lib/ecto_libsql.ex (1)
handle_execute(148-160)lib/ecto_libsql/native.ex (5)
begin(629-639)prepare(752-760)close_stmt(862-864)execute_stmt(782-802)query_stmt(823-849)
⏰ 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: Elixir 1.18.0 / OTP 27.0 / macos-latest
- GitHub Check: Elixir 1.18.0 / OTP 27.0 / ubuntu-latest
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
.beads/issues.jsonl (1)
1-33: Issue metadata changes look consistent with the implemented features.The updated JSONL records for security tests, named parameters, STRICT tables, RANDOM ROWID, and binary round‑trip fixes align with the new tests and adapter changes in this PR. No code changes here, so nothing to block on.
test/ecto_migration_test.exs (1)
14-32: Repo teardown and new migration-feature tests look solid.Stopping
TestRepoinon_exit, adding a short sleep, and cleaning-journalalongside-shm/-walshould help avoid file‑handle races between tests. The new migration tests forrandom_rowid: true,strict: true, and generated columns (virtual vs stored, plus invalid default/PK combinations) validate the SQL emitted byConnection.execute_ddl/1without forcing execution on older libSQL versions, which is a pragmatic balance. These changes align well with the migration and security guidance captured in your AGENTS notes.Also applies to: 771-882
| try do | ||
| EctoLibSql.handle_execute(sql, [value], [], state) | ||
| rescue | ||
| _e -> {:error, :exception} | ||
| _ -> {:error, :exception} | ||
| end | ||
|
|
||
| case result do | ||
| {:ok, _query, _result, _state} -> assert true | ||
| {:error, _reason} -> assert true | ||
| {:ok, _, _, _} -> assert true | ||
| {:error, _} -> assert true | ||
| end |
There was a problem hiding this comment.
Fix tuple arity mismatches on handle_execute/4 error paths in fuzz tests.
In the “extreme integer values”, “special float values”, and both BLOB fuzz properties, EctoLibSql.handle_execute/4 returns 3‑tuples on failure ({:error, error, state} / {:disconnect, reason, state}), but the code either:
- Matches
{:error, _}(2‑tuple) againstresult, or - Uses a rescue branch
{:error, :exception}(2‑tuple) while thecaseexpects a 3‑tuple error.
These will raise CaseClauseError whenever handle_execute/4 returns an error rather than treating it as an acceptable fuzz outcome.
I’d normalise everything to the 3‑tuple form on errors from these calls:
Suggested fix for the affected fuzz properties
@@
- result =
- try do
- EctoLibSql.handle_execute(sql, [value], [], state)
- rescue
- _ -> {:error, :exception}
- end
-
- case result do
- {:ok, _, _, _} -> assert true
- {:error, _} -> assert true
- end
+ result =
+ try do
+ EctoLibSql.handle_execute(sql, [value], [], state)
+ rescue
+ _ -> {:error, :exception, state}
+ end
+
+ case result do
+ {:ok, _, _, _} -> assert true
+ {:error, _, _} -> assert true
+ end
@@
- result =
- try do
- EctoLibSql.handle_execute(sql, [str_value], [], state)
- rescue
- _ -> {:error, :exception}
- end
-
- case result do
- {:ok, _, _, _} -> assert true
- {:error, _} -> assert true
- end
+ result =
+ try do
+ EctoLibSql.handle_execute(sql, [str_value], [], state)
+ rescue
+ _ -> {:error, :exception, state}
+ end
+
+ case result do
+ {:ok, _, _, _} -> assert true
+ {:error, _, _} -> assert true
+ end
@@
- result =
- try do
- EctoLibSql.handle_execute(sql, [blob_data], [], state)
- rescue
- _ -> {:error, :exception}
- end
-
- case result do
- {:ok, _, _, _} -> assert true
- {:error, _} -> assert true
- end
+ result =
+ try do
+ EctoLibSql.handle_execute(sql, [blob_data], [], state)
+ rescue
+ _ -> {:error, :exception, state}
+ end
+
+ case result do
+ {:ok, _, _, _} -> assert true
+ {:error, _, _} -> assert true
+ end
@@
- case EctoLibSql.handle_execute(insert_sql, [blob_data], [], state) do
+ case EctoLibSql.handle_execute(insert_sql, [blob_data], [], state) do
{:ok, _, _, new_state} ->
@@
- {:error, _} ->
+ {:error, _, _} ->
# Insert failed, that's acceptable for fuzz testing.
assert trueThis keeps the fuzz properties permissive (success or failure are both acceptable) while avoiding spurious crashes due to tuple shape mismatches.
Also applies to: 702-711, 726-735, 744-767
🤖 Prompt for AI Agents
In test/fuzz_test.exs around lines 682-691 (and similarly at 702-711, 726-735,
744-767), the fuzz tests assume 3-tuple error shapes from
EctoLibSql.handle_execute/4 but some branches return or match 2-tuples; change
the rescue so it returns a 3-tuple error (e.g., {:error, :exception, state}) and
update the case patterns to accept 3-tuple forms (match {:error, _, _} and
{:disconnect, _, _} alongside {:ok, _, _, _}); apply the same normalization to
the other listed line ranges so all failure paths consistently use 3-tuple
tuples and the case clauses match them.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.