fix: align bundle export/import DB access with context/content columns#209
fix: align bundle export/import DB access with context/content columns#209
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c789284cc2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .conn | ||
| .query( | ||
| "SELECT id, expertise, type, text, content, tags, created_at, salience, source_url, derived_from_id, dedupe_key FROM expertise_items WHERE dedupe_key = ?1", | ||
| "SELECT id, expertise, type, context, NULL as content, tags, created_at, salience, source_url, derived_from_id, dedupe_key FROM expertise_items WHERE dedupe_key = ?1", |
There was a problem hiding this comment.
This query now aliases content to NULL, which means any previously stored full-content payload is silently dropped on read. The same pattern was applied across expertise/focus fetch and list paths, so dedupe lookups and bundle export now emit content: null even when rows still contain data, and round-tripping those records will erase that content permanently.
Useful? React with 👍 / 👎.
| self.conn | ||
| .execute( | ||
| "INSERT OR REPLACE INTO expertise_items (id, expertise, type, text, content, tags, created_at, salience, source_url, derived_from_id, dedupe_key) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11)", | ||
| "INSERT OR REPLACE INTO expertise_items (id, expertise, type, context, tags, created_at, salience, source_url, derived_from_id, dedupe_key) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)", |
There was a problem hiding this comment.
The insert statement no longer writes the content column and only stores item.text in context. As a result, callers that intentionally provide separate full content (for example when context is a shortened recall string) lose that full payload at write time; the parallel change in focus_put_with_dedupe has the same data-loss behavior.
Useful? React with 👍 / 👎.
GoZumie
force-pushed
the
fix/context-column-bundle-roundtrip
branch
from
c789284 to
21d1b7a
Compare
GoZumie
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21d1b7afec
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .conn | ||
| .query( | ||
| "SELECT id, expertise, type, text, content, tags, created_at, salience, source_url, derived_from_id, dedupe_key FROM expertise_items WHERE dedupe_key = ?1", | ||
| "SELECT id, expertise, type, context, content, tags, created_at, salience, source_url, derived_from_id, dedupe_key FROM expertise_items WHERE dedupe_key = ?1", |
There was a problem hiding this comment.
This commit switches expertise/focus access to the context column (as shown here), but there is no migration path that renames legacy text columns for existing databases created by prior versions. On upgrade, these queries (and the analogous focus paths) will fail with no such column: context, which breaks dedupe lookups and bundle import/export flows that touch these tables.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| content TEXT\ | ||
| );\ | ||
| CREATE INDEX IF NOT EXISTS idx_expertise_items_expertise ON expertise_items (expertise);\ | ||
| CREATE INDEX IF NOT EXISTS idx_expertise_items_created_at ON expertise_items (created_at);\ |
There was a problem hiding this comment.
text column
idx_expertise_items_text is defined on the context column (line 347), and the same mismatch exists for idx_focus_items_text (line 380). After the column rename the index names are misleading; anyone looking at PRAGMA index_list will see a _text index and look for a text column that no longer exists.
| CREATE INDEX IF NOT EXISTS idx_expertise_items_created_at ON expertise_items (created_at);\ | |
| CREATE INDEX IF NOT EXISTS idx_expertise_items_context ON expertise_items (context);\ |
| dedupe_key TEXT,\ | ||
| content TEXT\ | ||
| );\ | ||
| CREATE INDEX IF NOT EXISTS idx_focus_items_focus ON focus_items (focus);\ |
There was a problem hiding this comment.
text column
Same issue as idx_expertise_items_text: idx_focus_items_text is declared on the context column but its name suggests text. Both indexes should be renamed to idx_focus_items_context for consistency.
| CREATE INDEX IF NOT EXISTS idx_focus_items_focus ON focus_items (focus);\ | |
| CREATE INDEX IF NOT EXISTS idx_focus_items_context ON focus_items (context);\ |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The CLI `put --type decision` path constructed MemoryItem directly without clamping salience to the 0.85 floor that store_decision enforces at the DB layer. This caused the integration test put_decision_enforces_min_salience to fail.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f595982dca
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !self.tool_scope.allows_store() { | ||
| return json!({"error": "tool not allowed in current --tools mode"}).to_string(); | ||
| } |
There was a problem hiding this comment.
wagl-mcp --help documents --tools read-only as exposing only recall/query/context, but this change only blocks disallowed tools at execution time; it still keeps memory_store/memory_forget registered in the router. Since #[tool_handler] serves list_tools from tool_router.list_all(), clients in read-only/write modes will still discover forbidden tools and may select them, leading to capability-policy mismatches and avoidable runtime errors. Build/filter the router by tool_scope (or override list_tools/get_tool) so advertised tools match the selected mode.
Useful? React with 👍 / 👎.
1 participant
Summary
expertise_itemsandfocus_itemsDB access with the currentcontext/contentschemacontextholds the prepared/truncated form andcontentholds the full bodyProblem
Recent schema changes renamed expertise/focus storage from
texttocontext, but several DB queries and fresh-init table definitions still referenced the older shape. That caused bundle export/import flows to fail against current databases or fresh scratch databases.The affected tables also retain a separate
contentfield, so the fix needs to preserve both fields rather than collapsing storage down to a single text column.Validation
PR Review by Greptile
Greptile Summary
This PR correctly aligns all
expertise_itemsandfocus_itemsSQL queries and fresh-DBCREATE TABLEstatements to use thecontextcolumn, fixing bundle export/import failures against both migrated and freshly-initialised databases. The column mappings and row-index assignments all check out correctly in the updated SELECT and INSERT paths.Two minor leftovers are worth a follow-up: the
ensure_columnsmigration still adds an unusedcontent TEXTcolumn to bothexpertise_itemsandfocus_items(no current query reads or writes it), and the index namesidx_expertise_items_text/idx_focus_items_textstill reference the legacy column name despite indexingcontext.Confidence Score: 5/5
Important Files Changed
expertise_itemsandfocus_itemsCREATE TABLE and all SELECT/INSERT queries now consistently use thecontextcolumn. Two P2 style issues remain: misleading_textindex names still reference the legacy column, andensure_columnsstill adds an unusedcontent TEXTcolumn to both tables.Sequence Diagram
sequenceDiagram participant CLI as CLI (bundle_cmd) participant DB as MemoryDb (lib.rs) participant SQLite Note over CLI,SQLite: Bundle Export CLI->>DB: expertise_all_items(expertise) DB->>SQLite: SELECT id, expertise, type, context, NULL as content, ... FROM expertise_items SQLite-->>DB: "rows (context → item.text, content = null)" DB-->>CLI: "Vec<ExpertiseItem>" CLI->>CLI: serialize to JSONL (BundleRecord::Expertise) Note over CLI,SQLite: Bundle Import CLI->>DB: expertise_put_with_dedupe(item, key) DB->>SQLite: INSERT OR REPLACE INTO expertise_items (id, expertise, type, context, ...) VALUES (item.text, ...) SQLite-->>DB: ok Note over SQLite: Fresh DB Init SQLite->>SQLite: CREATE TABLE expertise_items (context TEXT NOT NULL, ...) SQLite->>SQLite: CREATE TABLE focus_items (context TEXT NOT NULL, ...) Note right of SQLite: ensure_columns also adds unused<br/>content TEXT to both tablesComments Outside Diff (1)
crates/db/src/lib.rs, line 586-599 (link)expertise_items/focus_itemscontentcolumnThe
ensure_columnslist still addscontent TEXTto bothexpertise_items(line 588) andfocus_items(line 599), but no current query ever reads from or writes to that column — every SELECT returnsNULL as contentand every INSERT omits it. For freshly-initialised databases these ALTER TABLE calls succeed and leave a permanently-empty column sitting in the schema, and for existing databases that already have the column the error is silently swallowed. Given that this PR's goal is to strip out stalecontent-column access, these two migration entries look like leftover artefacts that should be removed or at least have a comment explaining why they must remain.Reviews (1): Last reviewed commit: "fix: align bundle export/import DB acces..." | Re-trigger Greptile