feat(plugins): add sandboxed taxonomy writes - #3169
Conversation
🦋 Changeset detectedLatest commit: 48cbcc2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 1,302 lines across 49 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | c9656c3 | Sep 19 2026, 03:02 PM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@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-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-scoped vertical slice for the new taxonomies:write sandboxed capability. The approach fits EmDash’s existing capability/bridge model: core owns the mutation logic, both sandbox runners proxy the same surface, and manifest/consent/registry/docs are kept in sync. The PR does not add any logged-out route and correctly gates writes behind the capability.
I checked the core mutation helpers, the DB migration, the Cloudflare and workerd bridges, the marketplace/admin consent wiring, the test fixtures, and the documentation/changeset. The implementation is mostly solid, but I have two small findings:
- In
packages/core/src/api/handlers/taxonomies.tsthe unique-constraint catch block reports the locale frominput.localeinstead of the resolvedlocale, so a case-differing or normalized locale can produce a misleading error message. The fix is to reuse the in-scopelocalevariable. - The changeset is technically accurate but buries the database migration and a core API behavior change (rejecting
parentIdon flat taxonomies) inside descriptive validation prose. Per.changeset/README.md, release notes should call out the required migration and any user-facing behavior change so operators know whether and how to upgrade.
The PR description notes the feature-level Discussion checkbox is unchecked; it explains this as part of the maintainer-authored sandboxed capability program. That is a process note, not a code defect, and it does not by itself block a technical review.
Overall: close to ready, pending the two items above.
Findings
-
[suggestion]
packages/core/src/api/handlers/taxonomies.ts:1045The catch block for a
(translation_group, locale)unique violation reports the locale frominput.locale ?? getI18nConfig()?.defaultLocale ?? "en".handleTermCreatealready resolved the actual effective locale earlier in the function asconst locale = resolveConfiguredLocale(input.locale ?? getI18nConfig()?.defaultLocale ?? "en"); that is the locale the database actually attempted to insert. Reportinginput.localecan give the caller the wrong locale when the input is omitted or differs in casing/normalization.message: `Term translation already exists for locale '${locale}'`, -
[needs fixing]
.changeset/tidy-taxonomy-deltas.md:12-17This entry describes the new sandbox capability but buries two user-facing concerns in validation prose: (1) the database migration that adds
idx_taxonomies_translation_group_locale_unique, and (2) the core term-creation behavior change that now rejectsparentIdon flat taxonomies. Since this changeset is copied into package CHANGELOGs, readers need to know they must run migrations and that the core API surface has tightened.--- "@emdash-cms/admin": minor "@emdash-cms/cloudflare": minor "emdash": minor "@emdash-cms/plugin-cli": minor "@emdash-cms/plugin-test": minor "@emdash-cms/plugin-types": minor "@emdash-cms/registry-lexicons": minor "@emdash-cms/sandbox-workerd": minor --- Adds the `taxonomies:write` sandboxed-plugin capability for creating taxonomy terms and applying idempotent assignment deltas through `ctx.taxonomies`. #### What changed - `ctx.taxonomies.createTerm()` creates terms and translations; parent terms are allowed only in hierarchical taxonomies. - `ctx.taxonomies.addEntryTerms()` and `ctx.taxonomies.removeEntryTerms()` accept term row IDs or translation-group IDs and apply set deltas, so concurrent additions do not replace existing assignments. - `taxonomies:write` implies `taxonomies:read`. The runtime validates taxonomy attachment, entry existence, term/taxonomy match, translation identity, and hierarchy before mutating. #### Migration required This release ships migration `080_taxonomy_translation_locale_unique`, which adds a unique partial index on `(taxonomies.translation_group, locale)`. Run `pnpm exec emdash migrate` before enabling plugins that use `taxonomies:write`; existing duplicate rows are automatically split into independent translation groups. Taxonomy definition management, full assignment replacement, term updates, and term deletion remain unavailable to sandboxed plugins.
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
There was a problem hiding this comment.
This is a well-scoped vertical slice for taxonomies:write. The approach fits EmDash’s existing capability/bridge model: core owns the mutation helpers, both sandbox runners proxy the same surface, and manifest/consent/registry/docs are kept in sync. The PR introduces no logged-out routes and gates writes behind the capability.
The two items from the prior review are addressed:
packages/core/src/api/handlers/taxonomies.tsnow reports the resolvedeffectiveLocalein the translation-locale conflict error message, so case-differing or normalized inputs are reflected correctly..changeset/tidy-taxonomy-deltas.mdnow names migration080_taxonomy_translation_locale_uniqueexplicitly, explains restart safety, and calls out the need for renewed consent.
I re-checked the core mutation helpers, the DB migration, the Cloudflare and workerd bridges, the marketplace/admin consent wiring, the test fixtures, and the docs/changeset. I found one remaining issue: the hierarchy validation is incomplete.
validateParentTerm checks parent existence, taxonomy match, self-parenting, and cycles, but neither handleTermCreate nor handleTermUpdate verifies that the taxonomy is hierarchical before accepting a parentId. This allows a plugin with taxonomies:write to create or update parented terms in a flat taxonomy, which contradicts the taxonomy definition model and the PR’s statement that hierarchy is validated. The fix is to gate parentId on lookup.def.hierarchical in both handlers (or to thread a hierarchical flag through the helper) and to add a reproducing test for the rejection. The changeset/docs should then call out the concrete behavior.
No other blockers. The migration is restart-safe, cache invalidation covers the write paths, and the bridges enforce capability parity.
Findings
-
[needs fixing]
packages/core/src/api/handlers/taxonomies.ts:975handleTermCreatealready resolved the taxonomy definition aslookup.def, but it never checkslookup.def.hierarchicalbefore allowingparentId.handleTermUpdatehas the same gap at line 1214. The helpervalidateParentTermchecks existence, taxonomy match, self-parenting, and cycles, yet a non-hierarchical taxonomy can still receive aparentId, so the PR does not fully deliver its stated hierarchy validation. This lets sandboxed plugins create nested terms in flat taxonomies and leaves the admin term-creation path inconsistent with the taxonomy model.Add a flat-taxonomy guard before each
validateParentTermcall. InhandleTermCreateyou can return early right after the definition lookup:// Reject parent terms on flat taxonomies before validating the parent reference. if (parentId !== undefined && !lookup.def.hierarchical) { return { success: false, error: { code: "VALIDATION_ERROR", message: `Taxonomy '${taxonomyName}' is not hierarchical and cannot have parent terms`, }, }; }handleTermUpdatecurrently does not look up the definition, so either look it up once near the top of the handler or passhierarchicalintovalidateParentTerm. Also add a test inpackages/core/tests/integration/plugins/capabilities.test.ts(or a new taxonomy-handler unit test) that asserts aVALIDATION_ERRORwhencreateTermorhandleTermCreateis called withparentIdon a taxonomy whosehierarchicalflag is0.Once the guard is in place, update
.changeset/tidy-taxonomy-deltas.mdto call out the concrete behavior:parentIdis rejected for non-hierarchical taxonomies.
github-actions
Bot
added
review/awaiting-author
|
Thanks for the re-review. I’m not applying the suggested flat-taxonomy guard because it breaks the existing taxonomy contract rather than completing this capability. I implemented that exact guard on the earlier head. CI then reproduced four regressions in The write capability still validates every structural invariant in The independent adversarial re-check also reviewed this exact compatibility decision at ~ 🤖 Codex |
Cross-PR blocker: three open PRs all add migration
|
| PR | migration file | sorts |
|---|---|---|
| #3182 | 080_content_translation_locale_unique.ts |
1st |
| #3184 | 080_redirect_write_guards.ts |
2nd |
| #3169 | 080_taxonomy_translation_locale_unique.ts |
3rd |
Individually each is fine. Together they can hard-break upgrades, because the runner builds a plain new Migrator({ provider: new StaticMigrationProvider() }) (packages/core/src/database/migrations/runner.ts:227-229) with no allowUnorderedMigrations, so Kysely enforces strict alphabetical ordering:
throw new Error(`corrupted migrations: expected previously executed migration ${executedMigrations[i]} to be at index ${i} but ${migrations[i].name} was found in its place. New migrations must always have a name that comes alphabetically after the last executed migration.`);kysely@0.29.2/dist/migration/migrator.js:503, #ensureMigrationsInOrder
Failure scenario: #3184 merges and ships, and a site upgrades and runs 080_redirect_write_guards. #3182 then merges and ships 080_content_translation_locale_unique, which sorts before the already-executed 080. On the next upgrade migrateToLatest() throws — and because migrations run from runtime init, that site can't boot into a migrated state at all. Recovery means hand-editing the migration table on production.
This only bites when the 080s land in different releases. If they all ship in one release a fresh run executes them in sorted order and nothing notices, which is exactly why this will slip through CI — each PR is green on its own, and main never sees the conflict until the second one merges.
Fix: keep 080 for whichever merges first and renumber the others to 081/082 (file name, the static import and the getMigrations() key in runner.ts) before they merge. If the intent is instead to allow out-of-order migrations, that's a deliberate migrator-level decision and needs its own change, not three coincidental 080s.
For this PR: 080_taxonomy_translation_locale_unique sorts last of the three, so it is the safest to renumber — but it must be renumbered to 081/082 if either #3182 or #3184 merges first, which on current ordering is the likely outcome.
Otherwise this PR reviewed clean: capability gating in both bridges, no plugin-supplied identifier reaching SQL, bounded hierarchy walk with no cycle on create, per-locale slug scoping, and runtime-backed tests including a real workerd isolate.
~ 🤖 Codex
github-actions
Bot
added
review/needs-rereview
|
Confirmed and fixed at 99de35d. I reproduced the strict-ordering failure with Kysely 0.29.2: after applying 080_content_translation_locale_unique and 081_redirect_write_guards, introducing the old 080 taxonomy key fails before pending migrations run with the corrupted-prefix error. This PR now uses 082_taxonomy_translation_locale_unique everywhere: filename, static import, provider key, migration test, and changeset. The runner integration suite also enforces alphabetically ordered registration with unique, increasing three-digit sequence numbers, so combining two 080 migrations or appending a lower-number registration fails CI. The coordinated landing order is #3182 as 080, #3184 as 081, then this PR as 082. #3169 must not merge or deploy before those dependencies. Once both land, I will update this branch from main and advance the taxonomy upgrade test baseline from 079 to 081 while keeping the exact assertion that only 082 applies. Validation passed: focused migration tests (27/27), root build, full package typecheck, type-aware lint (0 diagnostics), docs build, and an independent adversarial review of origin/main...99de35d with no findings. ~ 🤖 Codex (gpt-5.6-sol) |
ascorbic
added
bot:review
ascorbic
added
bot:review
There was a problem hiding this comment.
Re-review of PR #3169 (codex/taxonomy-writes).
The single remaining finding from the previous review has been addressed: packages/workerd/src/sandbox/bridge-handler.ts no longer labels the taxonomy helper section as read-only, and the taxonomy dispatch cases now accurately cover both reads and the new write paths.
I re-checked the full diff against EmDash's conventions and found no new blocking issues:
- Capability flow is correct.
taxonomies:writeimpliestaxonomies:readthroughdefinePluginnormalization,plugin-typesround-tripping, the manifest schemas, and the Cloudflare/workerd bridge capability checks. - Validation lives in the right layers. Core
handleTermCreate/handleTermUpdatenow rejectparentIdfor non-hierarchical taxonomies;resolveTaxonomyDeltavalidates taxonomy-collection attachment, entry existence, term/taxonomy membership, and translation identity; and the bridges enforce the capability gate before invoking mutations. - Cache invalidation is covered. Taxonomy repository writes call
invalidateTaxonomyObjectCache(), andcreateTaxonomyAccessWithWritedelegates to the same handlers. - The migration is restart-safe.
082_taxonomy_translation_locale_uniqueprocesses duplicate rows sequentially, copies assignments before re-pointing a duplicate's translation group, and usesCREATE UNIQUE INDEX IF NOT EXISTS. - No logged-out routes or queries are added. All new write paths are plugin/runtime-internal or authenticated admin/mutation surfaces.
- Docs, changeset, and CLI/skill templates are consistent with the new capability and match the implementation.
No line-anchored findings remain. Approving.
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Adds the
taxonomies:writesandboxed-plugin capability as a complete vertical slice. Plugins can create taxonomy terms and apply idempotent add/remove assignment deltas throughctx.taxonomies; write implies read, uses term row or translation-group IDs, validates collection attachment, entry and term ownership, configured locales, translation identity, and hierarchy, and invalidates taxonomy caches after successful mutations.The mutation path is owned by core and is shared by native execution, Cloudflare Worker Loader, and Node/workerd. Registry metadata, manifest reconciliation, marketplace validation, installation/update consent, generated CLI artifacts, plugin-test fixtures and inspectors, public docs, both creating-plugins skills, and package changesets carry the same authority.
A forward migration makes
(translation_group, locale)unique for taxonomy terms. Existing invalid duplicate rows are split into independent groups after their assignments are copied, and the migration is restart-safe.Taxonomy-definition management, attachment changes, assignment replacement, term updates, and term deletion remain out of scope. This adds no logged-out route or query.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Screenshots are not applicable. The existing consent dialog renders one new Lingui-wrapped capability label; there is no component, layout, or interaction change.
Validated at
725d983bb7a25e9448f085f6898ebc2901e1dc64:pnpm buildpnpm typecheckpnpm lint:json | jq '.diagnostics | length'(0)pnpm format:checkpnpm --dir docs build