feat: temporal scoring, staleness tracking, and drift detection#1257
feat: temporal scoring, staleness tracking, and drift detection#1257NoRain211 wants to merge 5 commits intothedotmack:mainfrom
Conversation
Greptile SummaryThis PR implements Ember MCP-inspired temporal memory management features. The implementation adds temporal decay scoring where memories naturally fade over time based on importance (half-life ranging from 18-180 days), access tracking that boosts frequently-retrieved memories, and staleness management to mark outdated observations. Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Search Request] --> B{ChromaDB Available?}
B -->|Yes| C[Vector Search]
B -->|No| D[SQLite FTS5 Search]
C --> E[Raw Results]
D --> E
E --> F[Apply Temporal Scoring]
F --> G{include_stale param?}
G -->|false default| H[Filter Stale Observations]
G -->|true| I[Keep All Results]
H --> J[Return Ranked Results]
I --> J
K[User Access] --> L[Track Access]
L --> M[Update last_accessed_at]
L --> N[Increment access_count]
O[Contradict Tool] --> P[Create Correction Observation]
P --> Q[Mark Original as Stale]
Q --> R[Link via corrected_by_id]
R --> S[Sync to ChromaDB]
T[Set Importance Tool] --> U[Clamp 1-10]
U --> V[Update importance field]
V --> W[Affects decay half-life]
X[Drift Check Tool] --> Y[Analyze Concepts via json_each]
Y --> Z[Calculate Stale %]
Z --> AA[Flag High-Stale Clusters]
AA --> AB[Return Drift Report]
Last reviewed commit: bbe926c |
| const tableInfo = this.db.query('PRAGMA table_info(observations)').all() as TableColumnInfo[]; | ||
| const hasColumn = tableInfo.some(col => col.name === 'last_accessed_at'); | ||
|
|
||
| if (!hasColumn) { | ||
| this.db.run('ALTER TABLE observations ADD COLUMN last_accessed_at INTEGER'); | ||
| this.db.run('ALTER TABLE observations ADD COLUMN access_count INTEGER NOT NULL DEFAULT 0'); | ||
| this.db.run('ALTER TABLE observations ADD COLUMN importance INTEGER NOT NULL DEFAULT 5'); | ||
| this.db.run('ALTER TABLE observations ADD COLUMN is_stale INTEGER NOT NULL DEFAULT 0'); | ||
| this.db.run('ALTER TABLE observations ADD COLUMN corrected_by_id INTEGER'); | ||
| logger.debug('DB', 'Added temporal scoring columns to observations table'); | ||
| } |
There was a problem hiding this comment.
Migration idempotency only checks for last_accessed_at column. If a partial migration occurs (e.g., first 2 columns added then a crash), retry will skip remaining columns.
| const tableInfo = this.db.query('PRAGMA table_info(observations)').all() as TableColumnInfo[]; | |
| const hasColumn = tableInfo.some(col => col.name === 'last_accessed_at'); | |
| if (!hasColumn) { | |
| this.db.run('ALTER TABLE observations ADD COLUMN last_accessed_at INTEGER'); | |
| this.db.run('ALTER TABLE observations ADD COLUMN access_count INTEGER NOT NULL DEFAULT 0'); | |
| this.db.run('ALTER TABLE observations ADD COLUMN importance INTEGER NOT NULL DEFAULT 5'); | |
| this.db.run('ALTER TABLE observations ADD COLUMN is_stale INTEGER NOT NULL DEFAULT 0'); | |
| this.db.run('ALTER TABLE observations ADD COLUMN corrected_by_id INTEGER'); | |
| logger.debug('DB', 'Added temporal scoring columns to observations table'); | |
| } | |
| const tableInfo = this.db.query('PRAGMA table_info(observations)').all() as TableColumnInfo[]; | |
| const existingColumns = new Set(tableInfo.map((col: any) => col.name)); | |
| if (!existingColumns.has('last_accessed_at')) { | |
| this.db.run('ALTER TABLE observations ADD COLUMN last_accessed_at INTEGER'); | |
| } | |
| if (!existingColumns.has('access_count')) { | |
| this.db.run('ALTER TABLE observations ADD COLUMN access_count INTEGER NOT NULL DEFAULT 0'); | |
| } | |
| if (!existingColumns.has('importance')) { | |
| this.db.run('ALTER TABLE observations ADD COLUMN importance INTEGER NOT NULL DEFAULT 5'); | |
| } | |
| if (!existingColumns.has('is_stale')) { | |
| this.db.run('ALTER TABLE observations ADD COLUMN is_stale INTEGER NOT NULL DEFAULT 0'); | |
| } | |
| if (!existingColumns.has('corrected_by_id')) { | |
| this.db.run('ALTER TABLE observations ADD COLUMN corrected_by_id INTEGER'); | |
| } | |
| logger.debug('DB', 'Added temporal scoring columns to observations table'); |
src/services/sqlite/SessionSearch.ts
Outdated
| let sql = ` | ||
| SELECT o.*, o.discovery_tokens |
There was a problem hiding this comment.
discovery_tokens already included in o.*
| let sql = ` | |
| SELECT o.*, o.discovery_tokens | |
| SELECT o.* | |
| FROM observations o |
Add Ember MCP-inspired features to claude-mem: **Temporal decay scoring:** - Observations decay in search relevance over time (importance-scaled half-life) - importance=1 → 18-day half-life, importance=5 → 90 days, importance=10 → 180 days - Access tracking bumps last_accessed_at + access_count on get_observations - Stale observations automatically excluded from search (opt-in via include_stale) **New MCP tools:** - `contradict` — mark a memory stale and record a correction linked to it - `set_importance` — adjust importance score (1-10) for decay tuning - `drift_check` — analyze concept clusters for semantic drift using SQL aggregation over json_each(concepts), flagging high-stale and likely-outdated clusters **SQLite FTS5 fallback:** - Restore text search when ChromaDB is unavailable (was returning empty) **Schema migration (v22):** - Adds last_accessed_at, access_count, importance, is_stale, corrected_by_id to observations table with idempotent column-existence check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bbe926c to
99eb54c
Compare
…ELECT - Check each temporal column individually before ALTER to handle partial migration failures (crash after adding some but not all 5 columns) - Remove redundant o.discovery_tokens from FTS5 SELECT (already in o.*) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@NoRain211 are you seeing improvements in performance with this PR? |
Chriscross475
left a comment
There was a problem hiding this comment.
Good implementation of temporal scoring and staleness tracking. Core logic is sound, but several issues need addressing:
Critical:
- Migration version mismatch: PR description says "migration v22" but code shows version 24 in
addTemporalScoringColumns(). Which is correct? This could cause migration conflicts.
Code Quality:
2. Silent error handling: FTS5 fallback in searchObservations() has try { ... } catch { return []; } with no logging. Errors will be invisible. Add:
} catch (err) {
logger.error('DB', 'FTS5 search failed', { query: ftsQuery }, err as Error);
return [];
}-
Type safety: Multiple
(obs as any).importance,(obs as any).access_count,(obs as any).is_stalecasts. Either:- Extend
ObservationSearchResultinterface with the new optional fields, OR - Add a comment explaining why
as anyis necessary (if fields aren't guaranteed to exist during migration)
- Extend
-
Test plan incomplete: All checkboxes unchecked. Please verify:
- Migration idempotency (run twice on same DB)
- Tools work end-to-end
- Temporal scoring doesn't crash on null values
Minor:
5. Drift detection: Returns generic message "Drift check unavailable" on any SQL error. Consider logging the error for debugging.
What's good:
- ✅ Temporal decay math is correct (verified 18d/90d/180d formula)
- ✅ Fire-and-forget access tracking is appropriate
- ✅ Input validation on API routes is solid
- ✅ Migration has column-existence checks (idempotent)
Fix the migration version and add error logging, then this is ready to merge.
These changes are about result quality, not query speed. The temporal scoring means I get more relevant context at session start — recent, frequently-accessed memories rank higher than stale ones from months ago. The practical improvement is spending less time sifting through outdated context and fewer cases where old architectural decisions get surfaced as current. I haven't benchmarked query latency since the scoring is applied in-memory after the SQLite/Chroma query returns — the overhead should be negligible for typical result sets (<100 observations). |
- Add temporal fields to ObservationRow interface, removing (obs as any) casts - Add error logging to FTS5 search catch block - Add error logging to drift detection catch block - Migration version is v24 (PR description will be updated) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Chriscross475
left a comment
There was a problem hiding this comment.
The changes look well-structured with good documentation and a comprehensive test plan. However, CI checks are not running on this PR.
Given that this PR includes:
- Database schema migration (v24 with 5 new columns)
- Complex temporal scoring logic
- FTS5 fallback implementation
I need to see CI passing before approval. Please:
- Ensure GitHub Actions workflow runs on this branch
- Verify all tests pass
- Confirm build succeeds with the new migration
Once CI is green, I'll do a full code review.
- Add rankByTemporalScore to SessionSearch mock (pass-through) - Update "empty results without Chroma" test to expect SQLite FTS5 fallback behavior instead of empty results Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI Verification (local)
BuildTests*The 1 server test failure is Test fixes in this push
Migration version noteThe PR description now correctly says v24 (not v22). v21-23 were taken by upstream commits added after our initial PR. |
When Chroma is configured but failing (backoff state), the fallback to SQLite was stripping the query text (query: undefined), making it impossible to do text search. Now preserves the query so FTS5 handles it instead of returning empty filter-only results. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Adds Ember MCP-inspired features for memory relevance management:
include_staleparam)contradicttool — mark a memory stale and record a correction linked to itset_importancetool — adjust importance score (1–10) to control decay ratedrift_checktool — SQL cluster analysis viajson_each(concepts)to detect concept areas where old memories have gone stale or been supersededSchema migration (v24)
Adds 5 columns to
observations:last_accessed_at,access_count,importance,is_stale,corrected_by_id. Idempotent with per-column existence checks before each ALTER to handle partial migration failures.Files changed
src/services/sqlite/migrations/runner.tssrc/services/sqlite/types.tsObservationRowinterfacesrc/types/database.tsObservationRecordsrc/services/sqlite/SessionSearch.tsrankByTemporalScore(importance-scaled),updateAccessTracking,detectDriftsrc/services/sqlite/SessionStore.tsmarkObservationStale,setObservationImportancesrc/services/worker/search/SearchOrchestrator.tssrc/services/worker/http/routes/MemoryRoutes.tssrc/services/worker/http/routes/DataRoutes.tssrc/servers/mcp-server.tscontradict,set_importance,drift_checkplugin/skills/mem-search/SKILL.mdTest plan
contradicttool: marks observation stale, creates correction, links viacorrected_by_idset_importancetool: clamps to 1–10, updates observationdrift_checktool: returns "no drift" on fresh DB, detects clusters after accumulationinclude_stale: true🤖 Generated with Claude Code