Add shared action foundation#30
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughIntroduces a shared "actions" layer in the CLI crate encapsulating remember, recall, export/import, audit, lifecycle (consolidate/maintain), adapter (DOX/Revolve sync), and integration scan operations. CLI and TUI now delegate to these actions. The SQLite crate splits internals into ChangesShared action foundation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant main
participant actions
participant SQLiteMemoryStore
User->>main: run command
main->>actions: build *ActionRequest
actions->>SQLiteMemoryStore: read/write as needed
SQLiteMemoryStore-->>actions: report
actions-->>main: *ActionReport
main->>main: print via commands::scriptable
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a shared action foundation for Tree Ring Memory, refactoring both the CLI and TUI to consume common action request and report contracts (such as remember, recall, export/import, audit, lifecycle, and adapters). Additionally, it simplifies the SQLite storage layer by splitting private helper functions into dedicated submodules (schema, write, search, and lifecycle) while preserving the public SQLiteMemoryStore facade. I have no feedback to provide as there are no review comments and the changes are well-structured and thoroughly tested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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 (2)
crates/tree-ring-memory-sqlite/src/lifecycle.rs (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
memory_ftsINSERT SQL acrosslifecycle.rsandwrite.rs.This statement mirrors the one in
write.rs'sput_with_statements/put_in_transaction. Consider extracting a shared constant or small helper for the FTS insert columns/statement to avoid schema drift between the two split modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tree-ring-memory-sqlite/src/lifecycle.rs` around lines 29 - 31, The `memory_fts` INSERT SQL is duplicated between `lifecycle.rs` and `write.rs`, which risks the two statements drifting apart. Extract the shared FTS insert statement or at least the common columns into a reusable constant/helper and have both `lifecycle.rs` and `put_with_statements`/`put_in_transaction` in `write.rs` use that single source of truth.crates/tree-ring-memory-cli/src/actions/export_import.rs (1)
65-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDry-run validation logic duplicates
SQLiteMemoryStore::import_jsonl's dry-run branch.The dry-run path here re-implements
decode_jsonl+normalize_import_events+ manualImportReportconstruction, which is functionally identical to the dry-run branch insideSQLiteMemoryStore::import_jsonl(see upstream contract snippet: builds the sameImportReportshape and returns early ondry_run). If the upstream dry-run logic ever changes (e.g. additional validation), this action wrapper will silently drift out of sync, causing CLI/TUI dry-run previews to diverge from the real import outcome.Consider extracting a shared "plan import" helper (in
tree-ring-memory-coreortree-ring-memory-sqlite) that both the store's dry-run branch and this store-less action wrapper delegate to.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tree-ring-memory-cli/src/actions/export_import.rs` around lines 65 - 92, The dry-run branch in import_jsonl is duplicating SQLiteMemoryStore::import_jsonl’s dry-run logic, so the CLI preview can drift from the real import behavior. Refactor the shared decode/normalize/report building into a common helper (for example a plan_import-style function in the core/sqlite layer) and have both import_jsonl in export_import.rs and SQLiteMemoryStore::import_jsonl call it. Keep the helper responsible for decode_jsonl, normalize_import_events, and constructing the ImportReport so both paths stay aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tree-ring-memory-cli/src/actions/export_import.rs`:
- Around line 135-159: The dry-run test in
import_action_dry_run_validates_without_writing does not exercise the actual
write path because import_jsonl is called with None, so target is never used.
Update the test to pass the created SQLiteMemoryStore into import_jsonl through
the shared import action path, then verify that the store remains empty after a
dry run. Keep the assertions tied to import_jsonl and ImportActionRequest so the
test validates the real no-write behavior rather than an untouched target.
In `@crates/tree-ring-memory-cli/src/actions/lifecycle.rs`:
- Around line 75-89: The maintain path in maintainer::maintain should not always
open SQLiteMemoryStore in read-only mode when store is None. Update the logic to
choose the connection mode based on request.dry_run: keep open_read_only for dry
runs, but open a writable store for apply mode before calling
store.maintain(&request). Make sure the behavior is handled in maintain and any
helper used by SQLiteMemoryStore so the None-store fallback cannot attempt
writes on a read-only connection.
In `@crates/tree-ring-memory-sqlite/src/schema.rs`:
- Around line 17-33: The URI built in open_read_only_connection is not
normalized correctly for Windows paths, so drive-letter paths and UNC paths can
be misinterpreted by SQLite. Update the path-to-URI handling in
open_read_only_connection, using normalize_sqlite_uri_path and sqlite_uri_path,
so drive-letter paths are prefixed with a leading slash and UNC paths are
handled separately before constructing the file: URI. Keep the existing
Connection::open_with_flags flow and adjust only the URI assembly logic to
preserve correct read-only behavior on Windows.
In `@crates/tree-ring-memory-sqlite/src/search.rs`:
- Around line 24-40: push_in_filter currently emits invalid SQL when values is
empty, so make the function defensive even if callers forget to guard. Update
push_in_filter in search.rs to check values.is_empty() up front and return
without modifying sql or parameters; otherwise keep the existing IN clause and
parameter population behavior unchanged.
---
Nitpick comments:
In `@crates/tree-ring-memory-cli/src/actions/export_import.rs`:
- Around line 65-92: The dry-run branch in import_jsonl is duplicating
SQLiteMemoryStore::import_jsonl’s dry-run logic, so the CLI preview can drift
from the real import behavior. Refactor the shared decode/normalize/report
building into a common helper (for example a plan_import-style function in the
core/sqlite layer) and have both import_jsonl in export_import.rs and
SQLiteMemoryStore::import_jsonl call it. Keep the helper responsible for
decode_jsonl, normalize_import_events, and constructing the ImportReport so both
paths stay aligned.
In `@crates/tree-ring-memory-sqlite/src/lifecycle.rs`:
- Around line 29-31: The `memory_fts` INSERT SQL is duplicated between
`lifecycle.rs` and `write.rs`, which risks the two statements drifting apart.
Extract the shared FTS insert statement or at least the common columns into a
reusable constant/helper and have both `lifecycle.rs` and
`put_with_statements`/`put_in_transaction` in `write.rs` use that single source
of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b545cfb-996b-4ad5-a7bb-9f24736051e5
📒 Files selected for processing (21)
.gitignorecrates/tree-ring-memory-cli/src/actions/adapters.rscrates/tree-ring-memory-cli/src/actions/audit.rscrates/tree-ring-memory-cli/src/actions/export_import.rscrates/tree-ring-memory-cli/src/actions/integrations.rscrates/tree-ring-memory-cli/src/actions/lifecycle.rscrates/tree-ring-memory-cli/src/actions/mod.rscrates/tree-ring-memory-cli/src/actions/recall.rscrates/tree-ring-memory-cli/src/actions/remember.rscrates/tree-ring-memory-cli/src/commands/mod.rscrates/tree-ring-memory-cli/src/commands/scriptable.rscrates/tree-ring-memory-cli/src/main.rscrates/tree-ring-memory-cli/src/tui/app.rscrates/tree-ring-memory-sqlite/src/lib.rscrates/tree-ring-memory-sqlite/src/lifecycle.rscrates/tree-ring-memory-sqlite/src/schema.rscrates/tree-ring-memory-sqlite/src/search.rscrates/tree-ring-memory-sqlite/src/write.rsdocs/architecture/rust-core-status.mddocs/superpowers/plans/2026-07-09-tree-ring-shared-action-foundation-implementation-plan.mddocs/superpowers/specs/2026-07-09-tree-ring-shared-action-foundation-design.md
| #[test] | ||
| fn import_action_dry_run_validates_without_writing() { | ||
| let dir = tempdir().unwrap(); | ||
| let mut source = SQLiteMemoryStore::open(dir.path().join("source.sqlite")).unwrap(); | ||
| let target = SQLiteMemoryStore::open(dir.path().join("target.sqlite")).unwrap(); | ||
| source | ||
| .put(&MemoryEvent::new("Import through shared action.", "lesson").unwrap()) | ||
| .unwrap(); | ||
| let (jsonl, _) = source.export_jsonl(false, false).unwrap(); | ||
| let input = dir.path().join("input.jsonl"); | ||
| fs::write(&input, jsonl).unwrap(); | ||
|
|
||
| let report = import_jsonl( | ||
| None, | ||
| ImportActionRequest { | ||
| path: input, | ||
| dry_run: true, | ||
| replace_existing: false, | ||
| }, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(report.report.valid_count, 1); | ||
| assert_eq!(target.list_all(true).unwrap().len(), 0); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test doesn't actually exercise the "no write" guarantee it claims to verify.
target is created but never passed into import_jsonl(None, ...) — the call always uses None for the store. So assert_eq!(target.list_all(true).unwrap().len(), 0) is trivially true regardless of dry-run behavior; target is untouched by the code under test. This gives false confidence that dry-run correctly avoids writes when a store is provided.
🧪 Suggested fix: exercise the store-provided dry-run path
- let report = import_jsonl(
- None,
- ImportActionRequest {
- path: input,
- dry_run: true,
- replace_existing: false,
- },
- )
- .unwrap();
-
- assert_eq!(report.report.valid_count, 1);
- assert_eq!(target.list_all(true).unwrap().len(), 0);
+ let mut target = target;
+ let report = import_jsonl(
+ Some(&mut target),
+ ImportActionRequest {
+ path: input,
+ dry_run: true,
+ replace_existing: false,
+ },
+ )
+ .unwrap();
+
+ assert_eq!(report.report.valid_count, 1);
+ assert_eq!(target.list_all(true).unwrap().len(), 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn import_action_dry_run_validates_without_writing() { | |
| let dir = tempdir().unwrap(); | |
| let mut source = SQLiteMemoryStore::open(dir.path().join("source.sqlite")).unwrap(); | |
| let target = SQLiteMemoryStore::open(dir.path().join("target.sqlite")).unwrap(); | |
| source | |
| .put(&MemoryEvent::new("Import through shared action.", "lesson").unwrap()) | |
| .unwrap(); | |
| let (jsonl, _) = source.export_jsonl(false, false).unwrap(); | |
| let input = dir.path().join("input.jsonl"); | |
| fs::write(&input, jsonl).unwrap(); | |
| let report = import_jsonl( | |
| None, | |
| ImportActionRequest { | |
| path: input, | |
| dry_run: true, | |
| replace_existing: false, | |
| }, | |
| ) | |
| .unwrap(); | |
| assert_eq!(report.report.valid_count, 1); | |
| assert_eq!(target.list_all(true).unwrap().len(), 0); | |
| } | |
| #[test] | |
| fn import_action_dry_run_validates_without_writing() { | |
| let dir = tempdir().unwrap(); | |
| let mut source = SQLiteMemoryStore::open(dir.path().join("source.sqlite")).unwrap(); | |
| let target = SQLiteMemoryStore::open(dir.path().join("target.sqlite")).unwrap(); | |
| source | |
| .put(&MemoryEvent::new("Import through shared action.", "lesson").unwrap()) | |
| .unwrap(); | |
| let (jsonl, _) = source.export_jsonl(false, false).unwrap(); | |
| let input = dir.path().join("input.jsonl"); | |
| fs::write(&input, jsonl).unwrap(); | |
| let mut target = target; | |
| let report = import_jsonl( | |
| Some(&mut target), | |
| ImportActionRequest { | |
| path: input, | |
| dry_run: true, | |
| replace_existing: false, | |
| }, | |
| ) | |
| .unwrap(); | |
| assert_eq!(report.report.valid_count, 1); | |
| assert_eq!(target.list_all(true).unwrap().len(), 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tree-ring-memory-cli/src/actions/export_import.rs` around lines 135 -
159, The dry-run test in import_action_dry_run_validates_without_writing does
not exercise the actual write path because import_jsonl is called with None, so
target is never used. Update the test to pass the created SQLiteMemoryStore into
import_jsonl through the shared import action path, then verify that the store
remains empty after a dry run. Keep the assertions tied to import_jsonl and
ImportActionRequest so the test validates the real no-write behavior rather than
an untouched target.
| pub fn maintain( | ||
| db_path: &Path, | ||
| store: Option<&mut SQLiteMemoryStore>, | ||
| request: MaintainActionRequest, | ||
| ) -> ActionResult<MaintenanceReport> { | ||
| let request = maintenance_request(request); | ||
| if request.dry_run && !db_path.exists() { | ||
| return Ok(plan_maintenance(&[], &request)); | ||
| } | ||
| if let Some(store) = store { | ||
| return store.maintain(&request).map_err(|err| err.to_string()); | ||
| } | ||
| let mut store = SQLiteMemoryStore::open_read_only(db_path).map_err(|err| err.to_string())?; | ||
| store.maintain(&request).map_err(|err| err.to_string()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
maintain opens read-only even when dry_run=false and no store is provided.
When store is None and request.dry_run is false (i.e., apply flags are set), the function unconditionally opens the database with open_read_only (line 87) but then calls store.maintain(&request) which will attempt writes. This will fail at runtime on a read-only SQLite connection.
While current CLI callers always pass Some(&mut store), the public signature permits store=None with apply flags set, making this a latent runtime failure path.
🔒 Proposed fix: conditionally open read-only vs read-write
if let Some(store) = store {
return store.maintain(&request).map_err(|err| err.to_string());
}
- let mut store = SQLiteMemoryStore::open_read_only(db_path).map_err(|err| err.to_string())?;
+ let mut store = if request.dry_run {
+ SQLiteMemoryStore::open_read_only(db_path)
+ } else {
+ SQLiteMemoryStore::open(db_path)
+ }
+ .map_err(|err| err.to_string())?;
store.maintain(&request).map_err(|err| err.to_string())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn maintain( | |
| db_path: &Path, | |
| store: Option<&mut SQLiteMemoryStore>, | |
| request: MaintainActionRequest, | |
| ) -> ActionResult<MaintenanceReport> { | |
| let request = maintenance_request(request); | |
| if request.dry_run && !db_path.exists() { | |
| return Ok(plan_maintenance(&[], &request)); | |
| } | |
| if let Some(store) = store { | |
| return store.maintain(&request).map_err(|err| err.to_string()); | |
| } | |
| let mut store = SQLiteMemoryStore::open_read_only(db_path).map_err(|err| err.to_string())?; | |
| store.maintain(&request).map_err(|err| err.to_string()) | |
| } | |
| pub fn maintain( | |
| db_path: &Path, | |
| store: Option<&mut SQLiteMemoryStore>, | |
| request: MaintainActionRequest, | |
| ) -> ActionResult<MaintenanceReport> { | |
| let request = maintenance_request(request); | |
| if request.dry_run && !db_path.exists() { | |
| return Ok(plan_maintenance(&[], &request)); | |
| } | |
| if let Some(store) = store { | |
| return store.maintain(&request).map_err(|err| err.to_string()); | |
| } | |
| let mut store = if request.dry_run { | |
| SQLiteMemoryStore::open_read_only(db_path) | |
| } else { | |
| SQLiteMemoryStore::open(db_path) | |
| } | |
| .map_err(|err| err.to_string())?; | |
| store.maintain(&request).map_err(|err| err.to_string()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tree-ring-memory-cli/src/actions/lifecycle.rs` around lines 75 - 89,
The maintain path in maintainer::maintain should not always open
SQLiteMemoryStore in read-only mode when store is None. Update the logic to
choose the connection mode based on request.dry_run: keep open_read_only for dry
runs, but open a writable store for apply mode before calling
store.maintain(&request). Make sure the behavior is handled in maintain and any
helper used by SQLiteMemoryStore so the None-store fallback cannot attempt
writes on a read-only connection.
| pub(crate) fn open_read_only_connection(path: &Path) -> TreeRingResult<Connection> { | ||
| let path = path | ||
| .canonicalize() | ||
| .map_err(|err| sqlite_error(err.to_string()))?; | ||
| let normalized_path = normalize_sqlite_uri_path(&path.to_string_lossy()); | ||
| let uri = format!( | ||
| "file:{}?mode=ro&immutable=1", | ||
| sqlite_uri_path(&normalized_path) | ||
| ); | ||
| let connection = Connection::open_with_flags( | ||
| uri, | ||
| OpenFlags::SQLITE_OPEN_READ_ONLY | OpenFlags::SQLITE_OPEN_URI, | ||
| ) | ||
| .map_err(sqlite_error_from_rusqlite)?; | ||
| configure_connection(&connection)?; | ||
| Ok(connection) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does SQLite require a leading slash before a Windows drive letter in a file: URI for it to be treated as an absolute path?
💡 Result:
Yes, SQLite requires a leading forward slash before a Windows drive letter in a file: URI for it to be treated as an absolute path [1][2][3]. When using the file: URI syntax to specify a database file on Windows, SQLite follows specific rules to interpret the path component [1]. If the authority section of the URI is omitted, the path is interpreted as an absolute path only if it begins with a "/" character [1][2]. For Windows specifically, if the path begins with "/X:/", where X is a drive letter, SQLite recognizes the "X:" as the drive volume rather than the top-level directory [4][1]. To correctly convert a Windows absolute filename into a URI, the official SQLite documentation recommends prepending a single forward slash to the path if it begins with a drive letter [1][2][3]. Additionally, all backslashes in the Windows path should be converted to forward slashes [1][2]. Failure to include the leading slash when the authority is omitted may result in the path being interpreted as a relative pathname [1][2].
Citations:
- 1: https://sqlite.org/draft/uri.html
- 2: https://www2.sqlite.org/uri.html
- 3: http://www3.sqlite.org/uri.html
- 4: https://sqlite.org/uri.html
🏁 Script executed:
git ls-files crates/tree-ring-memory-sqlite/src
sed -n '1,220p' crates/tree-ring-memory-sqlite/src/schema.rsRepository: TerminallyLazy/Tree-Ring-Memory
Length of output: 3847
🏁 Script executed:
rg -n "open_read_only_connection|open_read_only|audit|maintain|normalize_sqlite_uri_path|sqlite_uri_path" crates/tree-ring-memory-sqlite/srcRepository: TerminallyLazy/Tree-Ring-Memory
Length of output: 3858
🌐 Web query:
SQLite URI UNC path authority Windows SQLITE_ALLOW_URI_AUTHORITY server share file://server/share
💡 Result:
In SQLite, the authority component of a URI (e.g., in file://authority/path) is subject to strict validation by default [1][2]. Under normal circumstances, the authority must either be empty, omitted, or set to "localhost"; any other value will trigger an error [1][3]. To use a Windows UNC path (e.g., //server/share/file.db) within a SQLite URI, SQLite must be compiled with the SQLITE_ALLOW_URI_AUTHORITY preprocessor macro [1][2]. When this macro is enabled, any authority value other than "localhost" is treated as the server component of a UNC path and passed directly to the underlying operating system [1]. Key technical details regarding this behavior include: - Default Behavior: Without the SQLITE_ALLOW_URI_AUTHORITY macro, you cannot use arbitrary server names in the authority section of a URI [1][2]. - UNC Path Handling: When enabled, the authority becomes part of the UNC path string, allowing SQLite to interact with network shares [1][4]. - Security/Compatibility: This mechanism is intended to bridge the gap between standard URI syntax and Windows-specific network path conventions [1][2]. Note that even when using UNC paths, SQLite's performance over network shares (SMB) can be significantly affected by network latency and the locking semantics of the underlying file system, as SQLite relies on mandatory file locking to maintain database integrity [5]. If you are experiencing issues with UNC paths in libraries like System.Data.SQLite, ensure that your path strings are being correctly escaped or passed to the underlying SQLite API to avoid accidental path manipulation by the wrapper library [6].
Citations:
- 1: https://www2.sqlite.org/uri.html
- 2: https://sqlite.org/draft/uri.html
- 3: https://sqlite.org/matrix/matrix_duri.html
- 4: https://sqlite-users.sqlite.narkive.com/QXVZXFHl/3-7-7-uri-filenames-and-unc-paths
- 5: Is a SQLite connection over SMB without mounting possible? dotnet/efcore#31227
- 6: https://stackoverflow.com/questions/68914751/how-to-open-sqlite3-on-network-share-in-c-sharp-c-sharp
🏁 Script executed:
rg -n "SQLITE_ALLOW_URI_AUTHORITY|allow_uri_authority|uri_authority|sqlite3.*compile|rusqlite" .Repository: TerminallyLazy/Tree-Ring-Memory
Length of output: 9853
🏁 Script executed:
python3 - <<'PY'
def normalize_sqlite_uri_path(path: str) -> str:
if path.startswith(r"\\?\UNC\\"):
rest = path[len(r"\\?\UNC\\"):]
return r"\\\\" + rest.replace("\\", "/")
elif path.startswith(r"\\?\\" ):
rest = path[len(r"\\?\\"):]
return rest.replace("\\", "/")
else:
return path.replace("\\", "/")
def sqlite_uri_path(path: str) -> str:
out = []
for ch in path.encode():
if (65 <= ch <= 90) or (97 <= ch <= 122) or (48 <= ch <= 57) or ch in b"/-._~":
out.append(chr(ch))
else:
out.append(f"%{ch:02X}")
return "".join(out)
for sample in [r"C:\Users\lazy\memory.sqlite", r"\\?\C:\Users\lazy\memory.sqlite", r"\\?\UNC\server\share\memory.sqlite"]:
normalized = normalize_sqlite_uri_path(sample)
uri = f"file:{sqlite_uri_path(normalized)}?mode=ro&immutable=1"
print(sample)
print(" normalized:", normalized)
print(" uri:", uri)
PYRepository: TerminallyLazy/Tree-Ring-Memory
Length of output: 594
Windows read-only paths need URI normalization
open_read_only_connection turns C:\... into file:C%3A/..., which SQLite treats as a relative path. UNC paths become file://server/..., which also depends on URI-authority support. Prepend / for drive-letter paths and handle UNC separately before building the URI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tree-ring-memory-sqlite/src/schema.rs` around lines 17 - 33, The URI
built in open_read_only_connection is not normalized correctly for Windows
paths, so drive-letter paths and UNC paths can be misinterpreted by SQLite.
Update the path-to-URI handling in open_read_only_connection, using
normalize_sqlite_uri_path and sqlite_uri_path, so drive-letter paths are
prefixed with a leading slash and UNC paths are handled separately before
constructing the file: URI. Keep the existing Connection::open_with_flags flow
and adjust only the URI assembly logic to preserve correct read-only behavior on
Windows.
| pub(crate) fn push_in_filter( | ||
| sql: &mut String, | ||
| parameters: &mut Vec<Value>, | ||
| column_name: &str, | ||
| values: &[String], | ||
| ) { | ||
| sql.push_str(" AND "); | ||
| sql.push_str(column_name); | ||
| sql.push_str(" IN ("); | ||
| sql.push_str( | ||
| &std::iter::repeat_n("?", values.len()) | ||
| .collect::<Vec<_>>() | ||
| .join(", "), | ||
| ); | ||
| sql.push(')'); | ||
| parameters.extend(values.iter().cloned().map(Value::Text)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against empty values in push_in_filter.
If called with an empty slice, this generates invalid SQL (AND col IN ()). Current callers appear to guard before calling, but the function itself should be defensive against future misuse.
🛡️ Proposed fix
pub(crate) fn push_in_filter(
sql: &mut String,
parameters: &mut Vec<Value>,
column_name: &str,
values: &[String],
) {
+ if values.is_empty() {
+ return;
+ }
sql.push_str(" AND ");
sql.push_str(column_name);
sql.push_str(" IN (");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn push_in_filter( | |
| sql: &mut String, | |
| parameters: &mut Vec<Value>, | |
| column_name: &str, | |
| values: &[String], | |
| ) { | |
| sql.push_str(" AND "); | |
| sql.push_str(column_name); | |
| sql.push_str(" IN ("); | |
| sql.push_str( | |
| &std::iter::repeat_n("?", values.len()) | |
| .collect::<Vec<_>>() | |
| .join(", "), | |
| ); | |
| sql.push(')'); | |
| parameters.extend(values.iter().cloned().map(Value::Text)); | |
| } | |
| pub(crate) fn push_in_filter( | |
| sql: &mut String, | |
| parameters: &mut Vec<Value>, | |
| column_name: &str, | |
| values: &[String], | |
| ) { | |
| if values.is_empty() { | |
| return; | |
| } | |
| sql.push_str(" AND "); | |
| sql.push_str(column_name); | |
| sql.push_str(" IN ("); | |
| sql.push_str( | |
| &std::iter::repeat_n("?", values.len()) | |
| .collect::<Vec<_>>() | |
| .join(", "), | |
| ); | |
| sql.push(')'); | |
| parameters.extend(values.iter().cloned().map(Value::Text)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tree-ring-memory-sqlite/src/search.rs` around lines 24 - 40,
push_in_filter currently emits invalid SQL when values is empty, so make the
function defensive even if callers forget to guard. Update push_in_filter in
search.rs to check values.is_empty() up front and return without modifying sql
or parameters; otherwise keep the existing IN clause and parameter population
behavior unchanged.
1 participant
Summary
Verification
Certification Evidence
Review Notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores