fix: materialize plugin-declared storage indexes by edrpls · Pull Request #2308 · emdash-cms/emdash

edrpls · 2026-07-31T15:12:04Z

What does this PR do?

Fixes plugin-declared storage indexes never being created — and never being usable even if they had been.

The materialization machinery already exists (plugins/storage-indexes.ts, the _plugin_indexes tracking table from migration 004), but it has zero production callers: manifest storage.indexes declarations are consumed only as a query-validation allowlist. So PluginStorageRepository.query() ordering by a JSON field — e.g. the audit-log plugin's orderBy: { timestamp: "desc" } on its dashboard widget — runs as a full-table scan plus temp B-tree sort. Measured on the audited production deployment (Macabro festival site, emdash 0.31.1, 11 MiB _plugin_storage): 16.5–20.5 ms of synchronous main-thread work on every admin dashboard load, and _plugin_indexes empty.

Two coordinated changes:

Wire the sync into every plugin lifecycle moment. A new syncDeclaredStorageIndexes() (logs per-collection failures, never throws — a missing index is a performance problem, not a correctness one) runs on marketplace install/update, registry install/update, and index drops run on both uninstalls. Configured (in-config) plugins have no install handler at all, so they sync once per process on the scheduler tick — the cron path, never the request path, per the hot-path query rule.
Fix the index shape. The existing (never-invoked) generator built partial indexes (WHERE plugin_id = 'x' AND collection = 'y' via literals). Verified empirically: SQLite does not choose a partial index under bound parameters unless ANALYZE has run — and D1 never runs ANALYZE — so even wired up, the old shape would have indexed nothing in production. The new shape is a composite non-partial index (plugin_id, collection, json_extract(data, '$.field')), which SQLite selects in every scenario and which also eliminates the temp B-tree for ORDER BY. Unique-index semantics are preserved (uniqueness stays scoped per plugin+collection via the leading columns). No migration is needed: no deployed database has old-shape indexes, because nothing ever created them — that is this bug.

Postgres note: the composite index accelerates WHERE filters there, but not the jsonb ORDER BY (ordering uses -> for numeric-correct sorting while the index expression uses ->>; one index expression cannot match both). SQLite/D1 — where the measured problem lives — serves both. Repository query() semantics, results, and cursor behavior are unchanged; only the plan changes.

Found during a measured database audit of a production deployment.

Type of change

Checklist

I have read CONTRIBUTING.md
pnpm typecheck passes
pnpm lint passes
pnpm test passes (or targeted tests for my change)
pnpm format has been run
I have added/updated tests for my changes (if applicable)
User-visible strings in the admin UI are wrapped for translation (if applicable). Do not include messages.po changes except in translation PRs — a workflow extracts catalogs on merge to main. — n/a: no admin UI strings changed
I have added a changeset (if this PR changes a published package)
New features link to an approved Discussion — n/a: bug fix

AI-generated code disclosure

This PR includes AI-generated code — model/tool: Claude Fable 5 (Claude Code)

Screenshots / test output

Failing first (on main, before the fix) — the query-plan regression test reproduces the exact production plan:

× serves the repository's ORDER BY from the declared index without a temp B-tree
  + SEARCH _plugin_storage USING INDEX idx_plugin_storage_list (plugin_id=? AND collection=?)
  + USE TEMP B-TREE FOR ORDER BY

× materializes declared storage indexes on install and drops them on uninstall
× materializes indexes for every declared collection and is idempotent

After the fix — plugin/marketplace/registry suites all green, and the full packages/core suite matches the main baseline (the single virtual-modules.test.ts failure is pre-existing on a clean checkout in this environment — macOS temp-dir realpath mismatch):

Tests  803 passed (803)   (tests/integration/plugins/, tests/unit/plugins/, marketplace + registry handler suites)

🤖 Generated with Claude Code

The index-materialization module (storage-indexes.ts, _plugin_indexes tracking table) existed but had no production callers: manifest index declarations were only used as a query-validation allowlist, so every PluginStorageRepository.query ordering by a JSON field ran as a full scan plus temp B-tree sort — 16-20 ms of synchronous work per audit-log dashboard load on an audited 11 MiB production table. Two coordinated changes: - Wire syncStorageIndexes into every plugin lifecycle moment: marketplace and registry install/update create declared indexes, uninstall drops them, and configured (in-config) plugins — which have no install handler — sync once per process on the scheduler tick, off the request path. - Change the generated index shape from a partial index (WHERE plugin_id/collection literals) to a composite non-partial one (plugin_id, collection, expr...). SQLite never chooses the partial shape under bound parameters without ANALYZE statistics, which D1 never collects — so even wired up, the old shape indexed nothing in practice. Unique-index scoping is preserved by the leading columns. No deployed database has old-shape indexes (nothing ever created them). On Postgres the composite index accelerates WHERE filters but not the jsonb ORDER BY (the order expression uses '->' while the index uses '->>'); SQLite/D1 — where the measured problem lives — serves both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

changeset-bot · 2026-07-31T15:12:12Z

🦋 Changeset detected

Latest commit: e71eccb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages

Name	Type
emdash	Patch
@emdash-cms/cloudflare	Patch
@emdash-cms/sandbox-workerd	Patch
@emdash-cms/plugin-mcp-smoke	Patch
@emdash-cms/fixture-perf-site	Patch
@emdash-cms/perf-demo-site	Patch
@emdash-cms/cache-demo-site	Patch
@emdash-cms/do-demo-site	Patch
@emdash-cms/do-solo-demo-site	Patch
@emdash-cms/admin	Patch
@emdash-cms/auth	Patch
@emdash-cms/blocks	Patch
@emdash-cms/gutenberg-to-portable-text	Patch
@emdash-cms/x402	Patch
create-emdash	Patch
@emdash-cms/auth-atproto	Patch
@emdash-cms/plugin-embeds	Patch

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

pkg-pr-new · 2026-07-31T15:15:48Z

Open in StackBlitz

@emdash-cms/admin

npm i https://pkg.pr.new/@emdash-cms/admin@2308

@emdash-cms/auth

npm i https://pkg.pr.new/@emdash-cms/auth@2308

@emdash-cms/auth-atproto

npm i https://pkg.pr.new/@emdash-cms/auth-atproto@2308

@emdash-cms/blocks

npm i https://pkg.pr.new/@emdash-cms/blocks@2308

@emdash-cms/cloudflare

npm i https://pkg.pr.new/@emdash-cms/cloudflare@2308

@emdash-cms/contentful-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/contentful-to-portable-text@2308

emdash

npm i https://pkg.pr.new/emdash@2308

create-emdash

npm i https://pkg.pr.new/create-emdash@2308

@emdash-cms/gutenberg-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/gutenberg-to-portable-text@2308

@emdash-cms/plugin-cli

npm i https://pkg.pr.new/@emdash-cms/plugin-cli@2308

@emdash-cms/plugin-types

npm i https://pkg.pr.new/@emdash-cms/plugin-types@2308

@emdash-cms/registry-client

npm i https://pkg.pr.new/@emdash-cms/registry-client@2308

@emdash-cms/registry-lexicons

npm i https://pkg.pr.new/@emdash-cms/registry-lexicons@2308

@emdash-cms/registry-verification

npm i https://pkg.pr.new/@emdash-cms/registry-verification@2308

@emdash-cms/sandbox-workerd

npm i https://pkg.pr.new/@emdash-cms/sandbox-workerd@2308

@emdash-cms/x402

npm i https://pkg.pr.new/@emdash-cms/x402@2308

@emdash-cms/plugin-ai-moderation

npm i https://pkg.pr.new/@emdash-cms/plugin-ai-moderation@2308

@emdash-cms/plugin-atproto

npm i https://pkg.pr.new/@emdash-cms/plugin-atproto@2308

@emdash-cms/plugin-audit-log

npm i https://pkg.pr.new/@emdash-cms/plugin-audit-log@2308

@emdash-cms/plugin-color

npm i https://pkg.pr.new/@emdash-cms/plugin-color@2308

@emdash-cms/plugin-embeds

npm i https://pkg.pr.new/@emdash-cms/plugin-embeds@2308

@emdash-cms/plugin-field-kit

npm i https://pkg.pr.new/@emdash-cms/plugin-field-kit@2308

@emdash-cms/plugin-forms

npm i https://pkg.pr.new/@emdash-cms/plugin-forms@2308

@emdash-cms/plugin-webhook-notifier

npm i https://pkg.pr.new/@emdash-cms/plugin-webhook-notifier@2308

commit: e71eccb

emdashbot

This PR fixes a real, measured problem: plugin-declared storage indexes were never created, so plugin storage queries fell back to full scans. The approach is sound and fits EmDash’s architecture—lifecycle hooks in marketplace/registry handlers, plus one idempotent scheduler-time sync for configured plugins that have no install path. I focused on the SQL generation, the lifecycle wiring, and the tests.

Headline: the implementation is close, but one validation bug undermines the fix for valid plugin manifests. generateCreateIndexSql() validates the collection name with validateIdentifier(), which only allows /^[a-z][a-z0-9_]*$/. The manifest schema records storage: z.record(z.string(), …), and the repository treats collection as an opaque text value, so a perfectly valid collection name such as form-submissions (or any uppercase/kebab-case name) causes index creation to throw, get swallowed, and leave the plugin’s storage queries un-indexed. Because syncDeclaredStorageIndexes() logs and swallows errors, this fails silently—exactly the kind of gap the PR is trying to close.

A few inline comments also read like PR prose rather than evergreen code explanation and should be trimmed per AGENTS.md comment discipline.

The tests are generally good: they reproduce the production query plan, assert idempotency, cover unique indexes, and exercise the marketplace install/uninstall path. I didn’t run the suite (no shell), but the test cases look capable of failing on real regressions.

Findings

[needs fixing] packages/core/src/plugins/storage-indexes.ts:52

validateIdentifier(collection, "collection name") rejects collection names that the manifest schema and repository accept. The manifest declares storage: z.record(z.string(), …) and PluginStorageRepository uses the collection as a plain text value, so names like form-submissions are valid but will cause index creation to throw an IdentifierError, get swallowed by syncDeclaredStorageIndexes(), and leave the plugin without indexes.

The collection value is never interpolated into SQL as an identifier; it only appears inside the generated index name, which is safely quoted by sql.ref(...). Remove this overly strict validation.
```
	validatePluginIdentifier(pluginId, "plugin ID");
	for (const field of fields) {
		validateJsonFieldName(field, "index field name");
	}
```
[suggestion] packages/core/src/emdash-runtime.ts:704

This JSDoc contains reviewer-facing justification and an explicit "unlike X" comparison, which AGENTS.md calls out as comment discipline to avoid. The method name already conveys what it does; the remaining prose is a weaker version of the PR description and will stale quickly. Trim it to the useful invariant.
```
	/**
	 * Materialize plugin-declared storage indexes once per process.
	 *
	 * Called from the scheduler path, not from request handlers.
	 */
```
[suggestion] packages/core/src/api/handlers/marketplace.ts:482

Comment restates the call and its failure mode; the function name and JSDoc already communicate this. Remove to keep the call site clean.
```
		await syncDeclaredStorageIndexes(db, [bundle.manifest]);
```
[suggestion] packages/core/src/api/handlers/marketplace.ts:723

Comment is PR-prose justification; the surrounding update context and function name make the intent clear. Remove.
```
		await syncDeclaredStorageIndexes(db, [bundle.manifest]);
```
[suggestion] packages/core/src/api/handlers/marketplace.ts:799

Comment is a blend of restatement and reviewer-oriented rationale. The orphan-cost note is the only non-obvious point, but it belongs in the function JSDoc (or nowhere, since the orphaned-index cost is the whole reason for removeAllPluginIndexes). Keep the call clean.
```
		await removeAllPluginIndexes(db, pluginId);
```
[suggestion] packages/core/src/api/handlers/registry.ts:1188

Same as the marketplace install comment—restatement of the function call. Remove.
```
		await syncDeclaredStorageIndexes(db, [bundle.manifest]);
```
[suggestion] packages/core/src/api/handlers/registry.ts:1298

Same as the marketplace uninstall comment; keep the call site clean.
```
		await removeAllPluginIndexes(db, pluginId);
```
[suggestion] packages/core/src/api/handlers/registry.ts:1664

Comment justifies why the call exists; the function name and the surrounding update code explain this. Remove.
```
		await syncDeclaredStorageIndexes(db, [bundle.manifest]);
```

The manifest schema allows arbitrary storage collection keys and the repository stores collection as opaque text, but the index generator validated it as a SQL identifier — a kebab-case collection like form-submissions threw, the error was swallowed, and the plugin was left without indexes. The collection only appears inside the index name, which sql.ref quotes, so no identifier constraint applies. Also trims review-flagged call-site comments to keep the handlers clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

edrpls · 2026-07-31T15:45:08Z

Addressed in 8f27ad6:

Collection validation bug: confirmed and fixed exactly as you diagnosed — validateIdentifier(collection, ...) is removed from generateCreateIndexSql(); the collection only reaches SQL inside the index name, which sql.ref quotes. Added a regression test (accepts collection names that are not SQL identifiers) that fails on the previous commit with a swallowed IdentifierError for form-submissions and now passes with the index created and tracked.
Comment discipline: removed all five flagged call-site comments and trimmed the runtime JSDoc. I kept one sentence there — configured plugins have no install handler, so the scheduler tick is their only sync moment — since that's the non-obvious invariant a future reader needs to understand why this method exists at all.

Plugin/marketplace/registry suites: 176 passed.

Sandboxed marketplace/registry plugins load into the sandboxedPlugins map and never join allPipelinePlugins, so the once-per-process tick sync skipped them — plugins installed before this feature shipped would never get their declared indexes (the install/update handlers only cover new installs). Carry the manifest's storage declarations in the manifest cache populated at bundle load, and include them in the tick sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

edrpls · 2026-07-31T15:58:39Z

One more substantive fix in 83f894b, found by an adversarial review pass over this branch: the tick sync never covered sandboxed plugins. Marketplace/registry plugins in normal sandbox mode (the only mode on Cloudflare Workers) load into the sandboxedPlugins map and never join allPipelinePlugins, so plugins installed before this release would never get their indexes — the install/update handlers only cover future installs. Fixed by carrying storage declarations in the manifest cache (populated at bundle load, all four loader sites) and including cached manifests in the tick sync. Idempotent under the bypass-mode overlap.

Three smaller review findings I'm noting rather than fixing here, since all are pre-existing properties of the index machinery rather than this PR's wiring:

Removing a storage collection from a manifest between versions orphans that collection's indexes until full uninstall (syncStorageIndexes diffs within a collection, not across).
generateIndexName can collide (["a_b"] vs ["a","b"]), silently dropping one declaration.
A failed CREATE UNIQUE INDEX (e.g. pre-existing duplicate data) is logged-and-swallowed, which silently drops a declared uniqueness guarantee — arguably that one should fail the install, but that's a product decision.

Happy to file these as a follow-up issue or address them here if maintainers prefer.

The previous commit removed collection validation outright to unblock kebab-case names, which dropped the injection-shape rejection an existing unit test guards. sql.ref does neutralize such names — it doubles embedded quotes — but the defensive check is still worth keeping. Validate against the manifest key charset instead: letters, digits, underscores and hyphens, either case. form-submissions indexes, and quote/semicolon-bearing names are still rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

edrpls · 2026-07-31T16:15:06Z

CI caught that my earlier fix went too far — worth flagging since it touches the security posture.

Removing validateIdentifier(collection, ...) outright also removed the injection-shape rejection that tests/unit/plugins/storage-indexes.test.ts › should reject invalid collection names guards (items'; DROP TABLE--). I verified your reasoning holds — sql.ref does neutralize such names, doubling embedded quotes so the identifier stays inert:

CREATE INDEX "idx_""; DROP TABLE users; --" ON t(a)

so there was no exploitable path. But deleting a security test to make a change pass is the wrong resolution, so fc6d8e6 takes the middle road instead: a new validateStorageCollectionName using the manifest key charset (/^[a-zA-Z][a-zA-Z0-9_-]*$/) rather than SQL-identifier rules. form-submissions and formSubmissions now index; quote- and semicolon-bearing names are still rejected. Both the pre-existing security test and my kebab-case regression test pass unchanged — 181 tests green across the plugin/marketplace suites, and the full packages/core suite matches the main baseline.

github-actions Bot added the review/needs-review No maintainer or bot review yet label Jul 31, 2026

github-actions Bot added cla: signed area/core size/L labels Jul 31, 2026

emdashbot Bot reviewed Jul 31, 2026

View reviewed changes

edrpls mentioned this pull request Jul 31, 2026

plugin-audit-log: no retention path and full before+after snapshots make the audit trail grow without bound #2309

Open

github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/needs-review No maintainer or bot review yet labels Jul 31, 2026

edrpls force-pushed the fix/plugin-storage-indexes branch from fc6d8e6 to e71eccb Compare July 31, 2026 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: materialize plugin-declared storage indexes - #2308

fix: materialize plugin-declared storage indexes#2308
edrpls wants to merge 4 commits into
emdash-cms:mainfrom
edrpls:fix/plugin-storage-indexes

edrpls commented Jul 31, 2026 •

edited

Loading

Uh oh!

changeset-bot Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

pkg-pr-new Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

emdashbot Bot left a comment

Uh oh!

edrpls commented Jul 31, 2026

Uh oh!

edrpls commented Jul 31, 2026

Uh oh!

edrpls commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

edrpls commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

Checklist

AI-generated code disclosure

Screenshots / test output

Uh oh!

changeset-bot Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

pkg-pr-new Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emdashbot Bot left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

edrpls commented Jul 31, 2026

Uh oh!

edrpls commented Jul 31, 2026

Uh oh!

edrpls commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

edrpls commented Jul 31, 2026 •

edited

Loading

changeset-bot Bot commented Jul 31, 2026 •

edited

Loading

pkg-pr-new Bot commented Jul 31, 2026 •

edited

Loading