fix(core): let content:beforeSave hooks reject a save with an editor-facing message - #2617
Conversation
🦋 Changeset detectedLatest commit: 0410b36 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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
area/core
area/docs
size/L
review/needs-review
@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-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@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.
The approach is sound: this is the right change for the contract described in #2197. Introducing a dedicated ContentSaveRejectedError, catching it at the runtime's save entry points, and mapping it to a structured SAVE_REJECTED (422) envelope hides unexpected hook crashes behind CONTENT_HOOK_ERROR (500) while still letting trusted plugins show editors a clear message. It fits EmDash's handler/routing pattern and the existing admin API-client error path.
I checked the changed files, traced the two runContentBeforeSave call sites, verified the routes use unwrapResult (so mapErrorStatus maps the new codes to 422/500), and confirmed the admin's throwResponseError surfaces error.message to toasts. The tests correctly exercise success/rejection/crash paths for create and update.
Two non-blocking suggestions remain:
beforeSaveFailureuses raw string codes instead of theErrorCodeconstants the codebase provides. It's a convention drift in the handler layer and worth fixing while the code is new.- The new integration test's
afterEachcan crash with a secondary error ifboot()fails, becauseruntimeis stillundefined. A small guard makes failures easier to read.
Documentation and changeset are accurate and proportionate.
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. |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
I think this won't prevent the save: it just logs the error in runSandboxedBeforeSave and continues.
|
Docs narrowed and pushed: cancelling a save now says it needs the hook in the host isolate. That was wrong for sandboxed plugins before this PR too, and The trusted path does cancel the save: What that leaves is the part worth your judgement. Standard is the default format, and wherever a runner is configured — Otherwise I would do that path as a follow-up. Rethrowing in |
danielmlr
changed the title
…-facing message
A content:beforeSave hook is documented to cancel a save by throwing, but
handleContentCreate and handleContentUpdate did not catch the abort-policy
rethrow. The exception escaped the content routes, which have no try/catch,
so a cancelled save surfaced as an unstructured 500 instead of the
normal API error envelope.
Add ContentSaveRejectedError, exported from the package root. A trusted
hook that throws it now produces { code: "SAVE_REJECTED", message } with
HTTP 422, and the admin's existing save and autosave toasts show the
message. Any other exception from the hook pipeline is logged and mapped
to a generic CONTENT_HOOK_ERROR response, so plugin internals stay out of
API responses. The rejection is matched by error name as well as by
prototype because bundlers can duplicate the class across SSR chunks.
Sandboxed beforeSave hooks are unchanged: their errors are still logged
and the save proceeds. Letting sandboxed plugins reject saves needs an
error envelope across the sandbox RPC boundary, which is a separate
decision.
If boot() throws in beforeEach, runtime is still undefined and the unconditional stopCron() call reports a second teardown error on top of the real one. Optional chaining matches the teardown in media-usage-scheduled-driver.test.ts.
The route turns the runtime result into a response through mapErrorStatus, which recognizes SAVE_REJECTED by the value of ErrorCode.SAVE_REJECTED. A string literal in the runtime and that constant can drift apart, and the response then falls to the default 400 instead of 422. No test caught that, because the existing assertions compared the runtime literal against a test literal. Both SAVE_REJECTED cases now assert the mapped status as well. The reference table and the changeset name 422 for this error, so the status is part of the contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hooks page states that it covers sandboxed plugins, and runSandboxedBeforeSave logs a thrown error and continues, so the rejection contract never held there. Standard plugins run in an isolate on Cloudflare and in-process elsewhere, which makes the promise true in local development and false in production on Cloudflare. The reference page and the plugin-authoring skill document definePlugin, the standard format, and carried the same unqualified sentence.
runSandboxedBeforeSave logs a thrown error and continues, so the "throw to cancel" contract never held for sandboxed plugins. The hooks page under creating-plugins covers that format: the caution now says so, and the example no longer throws to cancel a save it cannot cancel. ContentSaveRejectedError is a runtime export of emdash while emdash/plugin carries types only, so a sandboxed plugin cannot import it either. 2f2d141 narrowed two further documents on the assumption that definePlugin is the sandboxed format. It is the native one: it requires id and version and runs in the host process, so the rejection works there as written. The hook reference and the plugin-authoring skill go back to the plain sentence, each with one clause on the sandboxed case.
"A sandboxed plugin cannot cancel a save" is false for a sandboxed plugin moved into plugins: [], which runs in the host process and takes the trusted hook path. The constraint belongs to the sandbox, not to the plugin format. Drop the throw from the skill reference's example: it sat directly under the sentence saying a plugin in the sandbox cannot cancel, and creating-plugins is read by sandboxed plugin authors. Sync the template copies, which had not been updated with the earlier docs commit on this branch.
The entry lands verbatim in the emdash CHANGELOG and carried the absolute that 003597d narrowed everywhere else.
5bf0db8 to
0d6c5dd
Compare
|
Rebased onto The rebase moved the
Nothing in the argument moves with them. The open question is still the last one in that comment: whether plugin-authored text may cross the sandbox boundary, given that routes carry a code and rebuild the message host-side, while a rejection has to reach an editor in the plugin's own words. |
A plugin developed with plugins: [] cancels a save by throwing; the same plugin running in the sandbox has its throw swallowed by runSandboxedBeforeSave, and the editor sees a successful save. The generic "hook error" line did not say that the cancel was lost, so the divergence between host and sandbox was silent. The log line now states that a sandboxed plugin cannot cancel a save and that the save continued. Detecting the rejection in that catch is not possible: the workerd wrapper turns a thrown error into a plain-text 500 body and the runner rethrows a new Error around it, and Workers RPC drops own properties such as the class-field name. Every throw from a sandboxed beforeSave therefore gets the same line.
|
The silent case is now loud: The boundary question from my first comment is withdrawn: a throw cannot carry the rejection out either way. The workerd wrapper turns it into a plain-text 500 ( One yes/no: merge this as the native-plugin half with that follow-up, or should I pull it until the sandbox path exists? |
There was a problem hiding this comment.
This is the right fix for the trusted-hook half of #2197: it introduces a dedicated ContentSaveRejectedError, catches it at the runtime's save entry points, maps it to a structured SAVE_REJECTED (422) envelope, and keeps unexpected hook crashes behind CONTENT_HOOK_ERROR (500). The new integration tests cover create, update, and the sandboxed case, and the docs/changeset explain the sandbox limitation accurately.
Since the last review, the two suggestions have been addressed: beforeSaveFailure now uses the ErrorCode constants instead of literal strings, and the new integration test's afterEach guards against runtime being undefined.
I did find one remaining gap: the OpenAPI generator for the content create/update endpoints still lists only 400/500 (and 404/409 for update). Because beforeSaveFailure can now return SAVE_REJECTED, which mapErrorStatus maps to 422, both endpoints can return a 422 response that isn't reflected in the machine-readable spec. The hand-written REST reference table was updated; the generated OpenAPI document should be too.
No logic bugs, security issues, or query-count regressions on logged-out routes.
Findings
-
[needs fixing]
packages/core/src/api/openapi/document.ts:258The content create endpoint can now return
422 SAVE_REJECTEDwhen a trustedcontent:beforeSavehook throwsContentSaveRejectedError, but the OpenAPI responses still only list400and500. The REST reference table indocs/src/content/docs/reference/rest-api.mdxwas updated to includeSAVE_REJECTEDat 422; the generated OpenAPI spec should match....standardErrors(400, 422, 500), -
[needs fixing]
packages/core/src/api/openapi/document.ts:316The content update endpoint can likewise return
422 SAVE_REJECTEDafter this change, but its OpenAPI responses omit 422. Add it alongside the existing 400/404/409/500 entries so the generated spec stays consistent with the implemented behavior....standardErrors(400, 404, 409, 422, 500),
The content create and update operations can return SAVE_REJECTED, which mapErrorStatus maps to 422, but their OpenAPI responses listed only 400/500 and 400/404/409/500. A client generated from the spec had no 422 case for either endpoint, while the hand-written REST reference table already documents one. standardErrors carries no 422 in its description map, so adding the code by itself would have emitted "Error 422" as the description.
There was a problem hiding this comment.
This re-review confirms the two needs_fixing items from the previous pass are resolved: packages/core/src/api/openapi/document.ts now lists 422 on both content create and update, and beforeSaveFailure uses the ErrorCode constants instead of string literals. The new integration test also guards afterEach against an undefined runtime.
The approach remains the right fix for the trusted-hook half of #2197. ContentSaveRejectedError is caught at the runtime's save entry points, mapped to a structured SAVE_REJECTED envelope with HTTP 422, and unexpected hook exceptions are masked as CONTENT_HOOK_ERROR (500) so internals don't leak. The sandbox limitation is documented accurately and the runtime now logs it explicitly without canceling the save.
I checked the full diff and conventions:
- Error responses follow the
ApiResult/unwrapResult/mapErrorStatusshape. - No new queries on logged-out routes.
- No SQL, auth, locale-filter, or index issues.
- The changeset is user-facing, proportional, and documents both behavior and the sandbox limitation.
- The admin client path (
lib/api/client.ts→ApiResponseError) already surfaceserror.messagein router toasts, so the plugin rejection message reaches the editor as claimed.
One non-blocking docs polish remains: the content:beforeSave prose tells plugin authors to throw ContentSaveRejectedError, but the accompanying example only demonstrates a slug transform. Adding a rejection branch would make the page demonstrate the capability it just described.
Findings
-
[suggestion]
docs/src/content/docs/plugins/creating-plugins/hooks.mdx:148-156The prose directly above the example tells plugin authors to throw
ContentSaveRejectedErrorto cancel a save, but the code block only demonstrates transforming a slug. Without a rejection branch, the page describes a new capability and then doesn't show readers how to use it. Consider adding a concrete rejection branch to the example so the prose and code agree."content:beforeSave": async (event, ctx) => { const { content, isNew } = event; if (typeof content.slug === "string") { content.slug = content.slug.toLowerCase().replace(/\s+/g, "-"); } if (isNew && !content.title) { throw new ContentSaveRejectedError("A post title is required."); } return content; },If you update this example, remember to re-run
scripts/sync-template-skills.shso the nine template copies stay in sync.
github-actions
Bot
added
review/awaiting-author
2 participants
What does this PR do?
Lets a
content:beforeSavehook cancel a save with a message the editor sees, but only for a plugin running in the host process. A sandboxed plugin cannot cancel. A throw reaches the host only as a plainError, so a rejection is indistinguishable from a crash; the save proceeds and the runtime logs why. Sandboxed is the format the plugin docs tell authors to default to, so this ships the capability for the other one. Closing that gap needs a different mechanism: a returned sentinel, the waycontent:beforeDeletealready readsreturn false. That is a follow-up, and whether the host-process half is worth merging without it is unresolved.Until now a cancelled save reached the client as an unstructured 500: the hook pipeline rethrows on the default
errorPolicy: "abort", and neither the runtime handlers nor the content routes caught it. ThrowingContentSaveRejectedError, exported from the package root, now produces{ code: "SAVE_REJECTED", message }with HTTP 422, and the admin's existing save and autosave toasts show the message to the editor. Any other exception is logged and mapped to a genericCONTENT_HOOK_ERROR, keeping hook internals out of API responses. The REST reference error table and the generated OpenAPI document both carry that 422 on content create and update.The hooks docs and the plugin-authoring skill state the sandbox limitation;
scripts/sync-template-skills.shcopies the skill into nine templates, which is why ten near-identical files change. Integration tests cover create, update and the sandboxed case.Part of #2197 — the host-process half only, so it does not close it. The publish path @virafb reported on 2026-08-27 is untouched:
handleContentPublishnever calls the hook.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. (n/a: no admin changes)AI-generated code disclosure
Screenshots / test output