feat: daily narrative memory tables + wagl day CLI (#120)#128
Conversation
There was a problem hiding this comment.
💡 Codex Review
Lines 815 to 819 in 13b3dc5
This merge keeps id, created_at, tags, and files from the existing row, but it takes environment and origin_domain only from the incoming item. Because the new provenance fields are optional on the HTTP API (PutItemRequest) and other callers can omit them, any idempotent write with upsert=true will clear previously captured provenance for that dedupe key. That turns routine updates into silent metadata loss.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Self-review notes: ✅ |
13b3dc5 to
9ab3418
Compare
…ECT, upsert provenance) - Add CREATE INDEX on daily_entries.date in init schema and v1→v2 migration (prevents full-table scans as daily entries accumulate over time) - Add --date + --invalidate validation: return clear error instead of silently discarding --date (invalidation looks up by ID only) - Refactor query_recent_high_valence and query_open_todos to use MEMORY_ITEM_SELECT constant instead of hardcoded column lists (prevents future divergence) - Fix upsert provenance: preserve environment/origin_domain from existing row when incoming item does not supply them (prevents silent metadata loss on idempotent writes via MCP/server that omit optional provenance fields) - Add TODO comment for link_daily_memory in Day --update handler (scaffolded but not yet called; needs content-similarity lookup to be built first) Closes remaining review items from Greptile (3/5→4/5) and Codex P2 flag.
|
Addressed all review feedback in commit c3b141a: Fixed:
Not fixed (intentional):
CI should be green. Ready for merge when you are. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b141a5bb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🔍 Review Status (2026-03-24)CI is all green ✅ and the branch is mergeable, but holding on the following before proceeding: 🔴 P1 — Bundle data-loss regression 🟡 P2 — Missing index on 🟡 P2 — 🟡 P2 — Will hold merge until at minimum the P1 bundle issue is resolved. Happy to delegate the fix to CCA. |
…128 P1) Closes the data-loss regression identified in PR #128 review: - Add DailyEntry + DailySummary variants to BundleRecord (wagl.bundle.daily_entry / wagl.bundle.daily_summary) - Add MemoryDb::all_daily_entries() and all_daily_summaries() for bulk export - Add MemoryDb::upsert_daily_entry() and upsert_daily_summary() for idempotent import (INSERT OR IGNORE by id / date) - Update export_bundle() to write all daily entries (including invalidated) and summaries - Update import_bundle() to restore daily entries and summaries; entries drive summary content via refresh_daily_summary, summary import restores the hand-authored summary field
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac863a8b4b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🔍 Review Update (2026-03-24 14:35 CST)P1 bundle data-loss regression → ✅ RESOLVED in The latest commit adds CI: All checks green ✅ (CodeQL, fmt + test, CI pipeline) Remaining P2 items (non-blocking):
These are cleanup items, not correctness issues. Can be followed up in a separate PR. @Zumie-BI — CI is green, P1 resolved. Ready for your approval if the feature looks good. 🚀 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ CI All Green — Ready to MergeAll checks passed on commit
P1 bundle data-loss regression resolved ✅ @Zumie-BI — PR is clean and ready. Approve when you're good. 🚀 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec67ed879
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let invalidated_i: i64 = if entry.invalidated { 1 } else { 0 }; | ||
| self.conn | ||
| .execute( | ||
| "INSERT OR IGNORE INTO daily_entries (id, date, context, content, invalidated, invalidate_reason, created_at) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)", |
There was a problem hiding this comment.
Upsert existing daily entries during bundle import
upsert_daily_entry uses INSERT OR IGNORE, so importing a newer bundle snapshot does not apply later changes to an existing entry (notably invalidated/invalidate_reason). This breaks incremental restore/sync workflows: an entry invalidated in the source DB remains active in the target DB after re-import, and refresh_daily_summary then rebuilds daily.content from that stale active row.
Useful? React with 👍 / 👎.
What
Append-only daily narrative layer:
daily_entries,daily_memories(junction),daily(materialized summary).CLI:
wagl daywagl day— show today's narrativewagl day --date 2026-03-21— specific datewagl day --list— list all dates with entrieswagl day --update— add an entry with contextwagl day --invalidate <id> --reason '...'— soft-delete with audit trailwagl day --include-invalidated— show everything including soft-deletedSchema
daily_entries: append-only, soft-invalidation viainvalidated_at+invalidation_reasondaily_memories: links entries to memory_itemsdaily: materialized summary per dateWhy
Multiple sessions (Becoming, Building, work) write memories throughout the day. The daily table provides a temporal narrative view without mutating individual items.
Closes #120
PR Review by Greptile
Greptile Summary
This PR introduces an append-only daily narrative layer (
daily_entries,daily_memories,daily) as part of the v1→v2 schema migration, and exposes it via the newwagl dayCLI subcommand with read, append, soft-invalidate, and list operations. It also threads two new provenance fields (environment,origin_domain) throughMemoryItem, all existing CLI write paths, the MCP handler, and the HTTP server — a clean, backward-compatible addition thanks to#[serde(default)]andOptiontyping.migrate.rs.memory_item_from_rowhelper eliminates ~120 lines of repeated row-deserialization across five query sites — a meaningful DRY improvement.daily_entries.datehas no index in either the init schema (lib.rs:489) or the migration (migrate.rs:220); queries filtering by date (get_daily_entries,invalidate_daily_entry's pre-lookup) will full-scan as the table grows over years of use.--datecombined with--invalidateis not caught byvalidate_day_args, so the date value is silently discarded rather than returning a clear error.link_daily_memory/daily_memoriesis scaffolded but never called from theDaycommand, leaving the described "links entries to memory_items" feature incomplete in this PR.query_recent_high_valence,query_open_todos_with_filter) were correctly updated to include the new columns but bypass theMEMORY_ITEM_SELECTconstant, creating a future divergence risk.Confidence Score: 4/5
daily_entries.dateindex and the--date/--invalidateguard; all other findings are non-blocking cleanups.memory_item_from_rowrefactor is a clear improvement. The missing index is a performance concern rather than a correctness bug, the silent--datediscard is a UX rough edge, and thedaily_memoriesgap is incomplete scaffolding — none of these break the primary user path. One targeted fix (the index) would make this a clean merge.crates/db/src/lib.rsandcrates/db/src/migrate.rsfor the missingdaily_entries.dateindex;crates/cli/src/main.rsfor the--date/--invalidatevalidation gap.Important Files Changed
environment/origin_domaintoMemoryItem, introducesmemory_item_from_rowhelper to eliminate duplicated row-mapping, and implements all five daily narrative methods (insert_daily_entry,get_daily_entries,get_daily_summary,list_daily_dates,invalidate_daily_entry,link_daily_memory,refresh_daily_summary). Missing index ondaily_entries.date; two temporal-recall queries bypass the newMEMORY_ITEM_SELECTconstant;link_daily_memoryis never called.migrate_v1_to_v2_innerwhich creates the three daily tables inside a transactional wrapper. Migration tests are updated and a newmigration_v1_to_v2_adds_daily_tablestest is added. Missing index ondaily_entries.datein the migration DDL (matches init schema gap).Cmd::Daywith full argument parsing,validate_day_args,validate_day_date,today_local_date_string, anddefault_origin_domain. Threadsenvironment/origin_domainthrough all existingPut,Canon, and ingest call sites.--dateis not validated as incompatible with--invalidate, causing silent discard.environmentandorigin_domainoptional fields (with#[serde(default)]) toMemoryItem, and introducesDailyEntryandDailySummarystructs. Clean, backward-compatible change.environment/origin_domainfields when constructingMemoryIteminsiderun_ingest_transcripts. Minimal, correct change.environment: None, origin_domain: Noneto three test fixtures to satisfy the updatedMemoryItemstruct. No logic changes.environment: None, origin_domain: Nonein the MCP handler'sMemoryItemconstruction. Minimal, correct change.environmentandorigin_domaininPutItemRequestand passes them through toMemoryItem. Backward-compatible (both fields areOption).Sequence Diagram
sequenceDiagram participant User participant CLI as wagl CLI (main.rs) participant DB as MemoryDb (db/lib.rs) participant SQLite Note over CLI,SQLite: wagl day --update "Finished auth refactor" User->>CLI: wagl day --update <content> [--context <label>] CLI->>CLI: validate_day_args() CLI->>CLI: today_local_date_string() CLI->>DB: insert_daily_entry(date, context, content) DB->>SQLite: INSERT INTO daily_entries DB->>DB: refresh_daily_summary(date) DB->>SQLite: SELECT valid entries WHERE date=? DB->>SQLite: INSERT OR REPLACE INTO daily (preserves summary) DB-->>CLI: entry id CLI->>DB: get_daily_entries(date, false) CLI->>DB: get_daily_summary(date) CLI-->>User: JSON {action, id, date, entries, daily} Note over CLI,SQLite: wagl day --invalidate <id> --reason "..." User->>CLI: wagl day --invalidate <id> [--reason <text>] CLI->>CLI: validate_day_args() CLI->>DB: invalidate_daily_entry(id, reason) DB->>SQLite: SELECT date FROM daily_entries WHERE id=? DB->>SQLite: UPDATE daily_entries SET invalidated=1 DB->>DB: refresh_daily_summary(date) DB->>SQLite: INSERT OR REPLACE INTO daily DB-->>CLI: ok CLI-->>User: JSON {action, id, reason} Note over CLI,SQLite: wagl day [--date YYYY-MM-DD] User->>CLI: wagl day [--date <date>] CLI->>CLI: validate_day_date() if date provided CLI->>DB: get_daily_entries(date, include_invalidated) CLI->>DB: get_daily_summary(date) CLI-->>User: JSON {date, count, entries, daily}Comments Outside Diff (1)
crates/db/src/lib.rs, line 978-997 (link)MEMORY_ITEM_SELECTTwo queries —
query_recent_high_valence(line 982) andquery_open_todos_with_filter(line 1025) — still hardcode the full column list instead of using the newMEMORY_ITEM_SELECTconstant. Both were manually updated to includeenvironment, origin_domain, which is correct for now, but they will silently diverge ifMEMORY_ITEM_SELECTis changed in the future. The same applies to the analogous query at line 1025.Consider replacing both inline projections with
MEMORY_ITEM_SELECT:Last reviewed commit: "feat: add environmen..."