Skip to content

fix(storage): repair skills_fts indexing on legacy databases - #2504

Open
zichen0116 wants to merge 1 commit into
MemTensor:mainfrom
zichen0116:fix/skills-fts-migration-backfill
Open

zichen0116 wants to merge 1 commit into
MemTensor:mainfrom
zichen0116:fix/skills-fts-migration-backfill

Conversation

@zichen0116

Copy link
Copy Markdown

Description

Fixes #2500: on databases created before the skills_fts FTS5 sync triggers landed in 001-initial.sql, the skills table has no skills_fts_ai / _ad / _au triggers — 001 never re-runs on existing databases, and unlike policies FTS (which got repair migration 009-policies-fts.sql) no later migration ever added them. Result: every INSERT into skills (including via importBundle()) leaves skills_fts empty, so the keyword channel in skill retrieval (WHERE skills_fts MATCH ... in core/retrieval/tier1-skill.ts) never matches — exactly the reported symptom (policies/world-model FTS populated, skills FTS at 0).

Implementation approach:

  • apps/memos-local-plugin/core/storage/migrations/020-skills-fts.sql: creates the skills_fts virtual table and the three sync triggers with IF NOT EXISTS, then backfills SELECT id, name, invocation_guide FROM skills WHERE id NOT IN (SELECT skill_id FROM skills_fts) — same shape and repair pattern as 009.
  • apps/memos-local-plugin/core/storage/migrator.ts: guard 020 with tableExists(db, "skills") so partial-schema test harnesses don't fail boot (same guard style as 009/012/018).

The migration is a no-op on fresh databases (001 already created everything there).

Related Issue (Required): Fixes #2500

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Unit Test — new regression test tests/unit/storage/skills-fts-migration.test.ts simulates a legacy DB (drops the triggers, empties FTS), reproduces the empty index, then applies the migration SQL: triggers restored, existing rows backfilled, trigram MATCH works, replay is idempotent, and subsequent insert/update/delete stay in sync; plus a fresh-DB no-op case.
  • Full run: npx vitest run tests/unit/storage/skills-fts-migration.test.ts tests/unit/storage/migrator.test.ts tests/unit/storage/fts-keyword.test.ts tests/unit/storage/keyword.test.ts → 36/36 passed; npm run lint (tsc --noEmit) clean.

Checklist

  • I have performed a self-review of my own code | 我已自行检查了自己的代码
  • I have commented my code in hard-to-understand areas | 我已在难以理解的地方对代码进行了注释
  • I have added tests that prove my fix is effective or that my feature works | 我已添加测试以证明我的修复有效或功能正常
  • I have created related documentation issue/PR in MemOS-Docs (if applicable) | N/A — internal schema repair, no user-facing behavior change beyond the bug being fixed
  • I have linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR(如果适用)

Copilot AI balanced review requested due to automatic review settings October 15, 2026 19:13
@Memtensor-AI Memtensor-AI added area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Oct 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2504
Task: 4c4c43d3b13ddcd2
Base: main
Head: fix/skills-fts-migration-backfill

🔍 OpenCodeReview found 2 issue(s) in this PR.


1. apps/memos-local-plugin/core/storage/migrations/020-skills-fts.sql (L25-L28)

The backfill is presence-only: it inserts rows whose skill_id is absent from the index, but never refreshes rows that already exist. Since this migration exists precisely because legacy databases can have a skills_fts that is out of sync with skills (any rename/edit made while skills_fts_au was missing leaves the old name/invocation_guide indexed), those drifted rows survive the repair and searchByText keeps returning stale matches — the exact failure this file is meant to fix. Duplicate rows left behind by the same broken trigger state are likewise never collapsed.

NOT IN also has a sharp edge here: if a single skills_fts.skill_id is NULL, three-valued logic makes the predicate never true and the backfill silently indexes nothing (a rebuild avoids the question entirely).

A full rebuild is cheap for a skills-sized table and makes the repair total — it re-derives the index from the source of truth, clearing stale, duplicate and orphan rows in one shot. Note INSERT OR REPLACE is not an option: FTS5 has no unique constraint on skill_id, so it would append duplicates instead of reconciling.

💡 Suggested Change

Before:

INSERT INTO skills_fts(skill_id, name, invocation_guide)
SELECT id, name, invocation_guide
  FROM skills
 WHERE id NOT IN (SELECT skill_id FROM skills_fts);

After:

DELETE FROM skills_fts;
INSERT INTO skills_fts(skill_id, name, invocation_guide)
SELECT id, name, invocation_guide
  FROM skills;

2. apps/memos-local-plugin/core/storage/migrator.ts (L244-L249)

The tableExists + db.exec guard duplicates the execIfTable(db, table, sql) helper defined in this same file (used by ensureNamespaceVisibilityColumns). Consider reusing it so the "only run when the parent table exists" rule has a single implementation.

if (file.version === 20 && file.name === "skills-fts") {
  execIfTable(db, "skills", fs.readFileSync(file.fullPath, "utf8"));
  return;
}
💡 Suggested Change

Before:

  if (file.version === 20 && file.name === "skills-fts") {
    if (tableExists(db, "skills")) {
      db.exec(fs.readFileSync(file.fullPath, "utf8"));
    }
    return;
  }

After:

  if (file.version === 20 && file.name === "skills-fts") {
    execIfTable(db, "skills", fs.readFileSync(file.fullPath, "utf8"));
    return;
  }

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

⚠️ Automated Test Results: ENV ISSUE

The test environment encountered an issue that requires manual attention.

Details: Environment preparation failed before any gating tests executed. Failed scopes: memos_local_plugin
Branch: fix/skills-fts-migration-backfill

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

importBundle 导入 skills 时不会写 skills_fts,技能的关键词检索永远是空的

4 participants