Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions crates/sl-daemon/src/etl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,4 +291,21 @@ mod tests {
let recalled = store.recall("pagination", 5).expect("recall distilled facts");
assert!(!recalled.is_empty(), "distilled episodic facts should persist");
}

#[cfg(feature = "sqlite")]
#[test]
fn transform_file_does_not_duplicate_durable_facts_for_repeated_input() {
use session_ledger::SqliteMemoryStore;

let tmp = tempfile::tempdir().expect("tempdir");
let jsonl = write_fixture(tmp.path(), 1);
let out = tmp.path().join("out");
let store = SqliteMemoryStore::open(tmp.path().join("memory.db")).expect("open memory db");

transform_file(&jsonl, &out, Some(&store)).expect("first transform");
transform_file(&jsonl, &out, Some(&store)).expect("repeated transform");

let facts = store.recall("session/sess-0", 10).expect("recall durable facts");
assert_eq!(facts.len(), 3, "repeated watcher events must not duplicate durable facts");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Watcher fact idempotency

Goal: prevent repeated filesystem-event handling of unchanged transcript input from duplicating SQLite episodic facts.

Success: a focused ETL regression test fails before the fix, passes after it, and the daemon's relevant test suite remains green.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Research

- `crates/sl-daemon/src/watcher.rs` forwards each create or modify event for a transcript; FSEvents may report repeated events for unchanged input.
- `crates/sl-daemon/src/etl.rs::transform_file` invokes `compile_and_store` once per parsed session when SQLite memory is configured.
- `src/ports/sqlite_memory.rs::store` currently assigns an incrementing identifier, so reprocessing the same session/key/content creates another durable row.
- `DistillMemoryWriter` already constructs a stable key from session id and bundle kind. A deterministic identity over session id, key, and content can make the SQLite primary key idempotent without changing watcher timing or HTTP behavior.

The red regression test transformed one JSONL fixture twice and recalled six SQLite facts where the three stable distilled facts were expected. This confirms the duplicate originates at the durable SQLite identity boundary.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Specifications

Repeated handling of one unchanged transcript must leave one durable SQLite fact per distilled fact identity. Distinct content must remain independently persistable.

Scope excludes HTTP persistence, viewer changes, daemon configuration, and live daemon control.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Work breakdown

1. Trace watcher, ETL, memory writer, and SQLite store.
2. Add a focused failing repeated-input regression test.
3. Make durable identity idempotent and validate targeted tests.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Implementation strategy

Use a deterministic content identity for SQLite memory facts and SQLite primary-key conflict handling. This fixes the durable boundary where duplicates are created and avoids debouncing heuristics that could discard legitimate later edits.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Known issues

OS filesystem event timing is intentionally not used in the test; the deterministic repeated ETL input reproduces its downstream effect.

The current checkout's repository-wide `cargo fmt --check` reports pre-existing formatting drift in unrelated daemon resolver, HTTP, main, and lib files. The changed Rust files pass an isolated `rustfmt --check`. `cargo clippy` is blocked before linting by the existing `clippy.toml` fields `warn` and `allow`, which this installed Clippy rejects.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Testing strategy

Run a focused `sl-daemon` ETL unit test that transforms the same real JSONL fixture twice through a SQLite memory store, then run the daemon crate test suite with the SQLite feature.

Observed RED: 6 recalled durable facts after two identical transforms. Expected and observed GREEN: 3 facts.
34 changes: 22 additions & 12 deletions src/ports/sqlite_memory.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
//! SQLite-backed [`MemoryStore`] adapter for durable episodic facts.

Check failure on line 1 in src/ports/sqlite_memory.rs

View workflow job for this annotation

GitHub Actions / Trunk Check

rustfmt

Incorrect formatting, autoformat by running 'trunk fmt'
//!
//! Opens a database file, applies [`crate::schema::migrate::apply_all`], and
//! persists distilled facts in the versioned `memory_facts` table.

use std::path::Path;
use std::sync::atomic::{AtomicU64, Ordering};
use std::sync::Mutex;

use rusqlite::{params, Connection};
use sha2::{Digest, Sha256};

use super::{MemoryStore, PortError};
use crate::schema::migrate::{self, MigrateError};

/// Durable [`MemoryStore`] backed by `SQLite` with forward-only migrations.
pub struct SqliteMemoryStore {
conn: Mutex<Connection>,
next_id: AtomicU64,
}

impl SqliteMemoryStore {
Expand All @@ -31,12 +30,7 @@
.map_err(|error| map_sqlite(&error))?;
migrate::apply_all(&conn).map_err(|error| map_migrate(&error))?;

let count: i64 = conn
.query_row("SELECT COUNT(*) FROM memory_facts", [], |row| row.get(0))
.map_err(|error| map_sqlite(&error))?;
let next_id = u64::try_from(count).unwrap_or(0);

Ok(Self { conn: Mutex::new(conn), next_id: AtomicU64::new(next_id) })
Ok(Self { conn: Mutex::new(conn) })
}

/// Open an in-memory database for tests.
Expand All @@ -47,7 +41,7 @@
pub fn open_in_memory() -> Result<Self, PortError> {
let conn = Connection::open_in_memory().map_err(|error| map_sqlite(&error))?;
migrate::apply_all(&conn).map_err(|error| map_migrate(&error))?;
Ok(Self { conn: Mutex::new(conn), next_id: AtomicU64::new(0) })
Ok(Self { conn: Mutex::new(conn) })
}

/// Lightweight readiness probe for `/readyz` dependency checks.
Expand All @@ -65,8 +59,7 @@

impl MemoryStore for SqliteMemoryStore {
fn store(&self, session_id: &str, key: &str, content: &str) -> Result<String, PortError> {
let sequence = self.next_id.fetch_add(1, Ordering::Relaxed);
let id = format!("memory-{sequence:020}");
let id = fact_id(session_id, key, content);
let payload = serde_json::json!({
"key": key,
"content": content,
Expand All @@ -78,7 +71,9 @@
PortError::Backend(format!("sqlite memory store lock poisoned: {error}"))
})?;
conn.execute(
"INSERT INTO memory_facts (id, session_id, kind, payload_json) VALUES (?1, ?2, 'EPISODIC', ?3)",
"INSERT INTO memory_facts (id, session_id, kind, payload_json)
VALUES (?1, ?2, 'EPISODIC', ?3)
ON CONFLICT(id) DO NOTHING",
params![id, session_id, payload_json],
)
.map_err(|error| map_sqlite(&error))?;
Expand Down Expand Up @@ -129,6 +124,21 @@
}
}

fn fact_id(session_id: &str, key: &str, content: &str) -> String {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: Deterministic IDs change recall pagination semantics

fact_id generates SHA-256 hex IDs. The recall function's ORDER BY id ASC previously preserved insertion order because IDs were monotonically increasing. With hash-based IDs, ordering is now effectively random, which changes top_k pagination behavior for consumers expecting chronological results.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

let mut hasher = Sha256::new();
for value in [session_id, key, content] {
hasher.update(u64::try_from(value.len()).unwrap_or(u64::MAX).to_be_bytes());
hasher.update(value.as_bytes());
}
let digest = hasher.finalize();
let mut id = String::from("memory-");
for byte in digest {
use std::fmt::Write;
let _ = write!(id, "{byte:02x}");
}
id
}

fn map_sqlite(error: &rusqlite::Error) -> PortError {
PortError::Backend(format!("sqlite memory store: {error}"))
}
Expand Down
Loading