fix: correct migration rollback, media ID, MCP taxonomy tools, and FTS escaping#497
fix: correct migration rollback, media ID, MCP taxonomy tools, and FTS escaping#497
Conversation
…S query escaping - Migration 011 down(): drop own indexes (idx_sections_*) instead of indexes from migration 015 (H4) - Plugin media upload: return the DB-generated media ID instead of the orphaned ULID used only for the storage key prefix (H5) - MCP taxonomy tools: delegate to handler layer (handleTaxonomyList, handleTermList, handleTermCreate) instead of raw SQL with as-never casts. Gets slug validation, duplicate checking, and type safety (H6, M14) - FTS escapeQuery: check for operators before escaping quotes, not after. Previously, queries with both quotes and operators produced malformed FTS5 syntax (M19)
…gination
Addresses adversarial review:
- escapeQuery: restore quote escaping before returning on operator path.
The previous fix removed escaping, creating a regression where queries
with both quotes and operators were sent raw to FTS5.
- MCP taxonomy_list_terms: restore limit/cursor pagination and { items }
response shape. The handler layer (handleTermList) has no pagination
and runs N+1 queries, so keep paginated inline query via repository.
🦋 Changeset detectedLatest commit: 8885917 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 8885917 | Apr 13 2026, 06:39 PM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes multiple correctness issues across migrations, plugin media upload, MCP taxonomy tools, and FTS query escaping.
Changes:
- Corrects migration
011_sectionsrollback to drop the proper indexes. - Fixes plugin media upload to return the DB-created media ID (not the storage key ULID).
- Refactors MCP taxonomy tools to use handler/repository layers and adjusts FTS query escaping behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/search/query.ts | Adjusts FTS5 query escaping logic (operators + quote escaping). |
| packages/core/src/plugins/context.ts | Returns DB media ID from upload flow; separates storage key prefix from media ID. |
| packages/core/src/mcp/server.ts | Delegates taxonomy list/create to handlers; refactors term listing to repository-based access. |
| packages/core/src/database/migrations/011_sections.ts | Fixes rollback to drop the correct indexes for migration 011. |
| .changeset/slow-flies-leave.md | Adds a patch changeset describing the fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core/src/search/query.ts
Outdated
| // If already quoted, pass through with quotes escaped | ||
| if (query.startsWith('"') && query.endsWith('"')) { | ||
| return query; | ||
| return escaped; |
There was a problem hiding this comment.
escapeQuery() treats already-quoted queries as needing quote escaping and returns escaped, but escaped is produced by replacing all " with "" (including the leading/trailing phrase delimiters). For an input like "hello world", this becomes ""hello world"", which changes/breaks FTS5 phrase syntax. Preserve the outer quotes and only escape interior quotes (e.g., strip the outer quotes first, escape the inner content, then re-wrap), or perform the already-quoted check before global quote escaping.
| // Verify taxonomy exists via handler layer | ||
| const { handleTaxonomyList } = await import("../api/handlers/taxonomies.js"); | ||
| const listResult = await handleTaxonomyList(ec.db); | ||
| if (!listResult.success) return unwrap(listResult); | ||
|
|
||
| const taxonomies = (listResult.data as { taxonomies: Array<{ name: string; id?: string }> }) | ||
| .taxonomies; | ||
| const taxonomy = taxonomies.find((t: { name: string }) => t.name === args.taxonomy); |
There was a problem hiding this comment.
taxonomy_list_terms verifies taxonomy existence by calling handleTaxonomyList() and then scanning the full list. This couples term listing to successfully parsing all taxonomy defs (including collections JSON) and can fail even when the requested taxonomy exists. Prefer a direct lookup (e.g., the requireTaxonomyDef logic used in handlers/taxonomies.ts) so term listing only depends on the requested taxonomy row.
| // Verify taxonomy exists via handler layer | |
| const { handleTaxonomyList } = await import("../api/handlers/taxonomies.js"); | |
| const listResult = await handleTaxonomyList(ec.db); | |
| if (!listResult.success) return unwrap(listResult); | |
| const taxonomies = (listResult.data as { taxonomies: Array<{ name: string; id?: string }> }) | |
| .taxonomies; | |
| const taxonomy = taxonomies.find((t: { name: string }) => t.name === args.taxonomy); | |
| // Verify only the requested taxonomy exists via direct handler lookup. | |
| // This avoids coupling term listing to parsing every taxonomy definition. | |
| const { handleTaxonomyGet } = await import("../api/handlers/taxonomies.js"); | |
| const taxonomyResult = await handleTaxonomyGet(ec.db, args.taxonomy); | |
| if (!taxonomyResult.success) return unwrap(taxonomyResult); | |
| const taxonomy = (taxonomyResult.data as { taxonomy?: { name: string; id?: string } }).taxonomy; |
| // Paginated term query via repository (avoids N+1 of handleTermList) | ||
| const { TaxonomyRepository } = await import("../database/repositories/taxonomy.js"); | ||
| const repo = new TaxonomyRepository(ec.db); | ||
| const limit = Math.min(args.limit ?? 50, 100); | ||
| let query = ec.db | ||
| .selectFrom("_emdash_taxonomy_terms" as never) | ||
| .selectAll() | ||
| .where("taxonomy_id" as never, "=", taxonomy.id as never) | ||
| .orderBy("label" as never, "asc") | ||
| .limit(limit + 1); | ||
| const terms = await repo.findByName(args.taxonomy); | ||
|
|
||
| // Manual cursor pagination over the sorted results | ||
| let startIdx = 0; |
There was a problem hiding this comment.
This pagination implementation loads all terms via repo.findByName() and then slices in memory. That defeats DB-level pagination and can become expensive for large taxonomies. Consider moving pagination into the repository/SQL query (e.g., ORDER BY label, id with a cursor encoding both fields, or switch ordering to id if you want to keep an id cursor) so the DB only returns limit + 1 rows.
| let media; | ||
| try { | ||
| await mediaRepo.create({ | ||
| media = await mediaRepo.create({ |
There was a problem hiding this comment.
let media; introduces an untyped local that becomes any, losing type safety around media.id. Prefer declaring it with a concrete type (e.g., let media: MediaItem) or restructure to keep const media = await mediaRepo.create(...) in scope for the return (e.g., return after the try/catch).
The global " → "" replacement was running before the already-quoted check, so input like "hello world" became ""hello world"", breaking FTS5 phrase syntax. Move the quoted-phrase check first and only escape interior quotes.
…S escaping (emdash-cms#497) * fix: correct migration rollback, media ID, MCP taxonomy tools, and FTS query escaping - Migration 011 down(): drop own indexes (idx_sections_*) instead of indexes from migration 015 (H4) - Plugin media upload: return the DB-generated media ID instead of the orphaned ULID used only for the storage key prefix (H5) - MCP taxonomy tools: delegate to handler layer (handleTaxonomyList, handleTermList, handleTermCreate) instead of raw SQL with as-never casts. Gets slug validation, duplicate checking, and type safety (H6, M14) - FTS escapeQuery: check for operators before escaping quotes, not after. Previously, queries with both quotes and operators produced malformed FTS5 syntax (M19) * fix: restore quote escaping on FTS operator path, restore MCP term pagination Addresses adversarial review: - escapeQuery: restore quote escaping before returning on operator path. The previous fix removed escaping, creating a regression where queries with both quotes and operators were sent raw to FTS5. - MCP taxonomy_list_terms: restore limit/cursor pagination and { items } response shape. The handler layer (handleTermList) has no pagination and runs N+1 queries, so keep paginated inline query via repository. * chore: add changeset for correctness bug fixes * fix: escape only interior quotes in FTS5 quoted phrases The global " → "" replacement was running before the already-quoted check, so input like "hello world" became ""hello world"", breaking FTS5 phrase syntax. Move the quoted-phrase check first and only escape interior quotes.
What does this PR do?
Fixes five correctness bugs identified in the code quality review (H4, H5, H6, M14, M19).
Migration 011 rollback (H4):
down()was dropping indexes from migration 015 (idx_content_taxonomies_term,idx_media_mime_type) instead of its own (idx_sections_category,idx_sections_source). Rolling back 011 would destroy unrelated indexes.Plugin media upload ID (H5):
createMediaAccessWithWrite.upload()generated a ULID for the storage key prefix but returned it asmediaId. The database record got a different ULID frommediaRepo.create(). Plugins storing the returnedmediaIdwould reference a non-existent record. Fix: return the DB-generated ID.MCP taxonomy tools (H6, M14):
taxonomy_listandtaxonomy_create_termused raw Kysely queries withas nevercasts, bypassing type safety.taxonomy_create_terminserted directly without slug validation or duplicate checking.taxonomy_listhad unguardedJSON.parse. Rewritten to delegate to handler layer (handleTaxonomyList,handleTermCreate) which provide validation, dedup, and proper error handling.taxonomy_list_termsretains its paginated inline query (handler has no pagination and N+1 queries) but uses the repository for type safety.FTS escapeQuery (M19): The operator detection check (
AND,OR,NOT,NEAR) was testing the raw query but returning the escaped string, producing malformed FTS5 when queries contained both quotes and operators. Reordered to escape quotes first, then check for operators and return the escaped result.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure