feat(plugins): add publication policy hooks - #3185
Conversation
Scope checkThis PR changes 2,486 lines across 77 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. |
🦋 Changeset detectedLatest commit: cc676a8 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
cla: signed
review/needs-review
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
Bot
added
review/awaiting-author
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. |
github-actions
Bot
added
review/needs-rereview
ascorbic
added
bot:review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This follow-on to #3162 is the right scoped addition: it gates publication, scheduling, and unpublication through capability-registered policy hooks, persists scheduler rejections as versioned dashboard notices, and protects visual-editor publish actions with renewable tokens, all without granting content read/write authority to policy plugins.
I re-checked the two concrete gaps from the prior review and both are resolved on this head:
packages/core/src/mcp/server.tsnow requires_revon thecontent_scheduletool and forwards it tohandleContentSchedule, so MCP scheduling has the same lost-update protection as REST scheduling, with tests inpackages/core/tests/integration/mcp/content-misc.test.ts.packages/core/src/visual-editing/toolbar.tsrenders all toolbar label strings fromToolbarLabels, andpackages/admin/src/locales/server.tsexposes Lingui message descriptors for every label, satisfying the admin-string localization requirement.
Beyond those fixes, I re-reviewed the policy implementation, runtime hooks, REST and MCP route origin/actor plumbing, scheduler rejection persistence and cleanup, dashboard/auth wiring, and the visual-editing token flow. No new logic bugs, security issues, data-loss concerns, AGENTS.md violations, or missing tests were found. The changeset is detailed and the docs match the implemented surface.
github-actions
Bot
added
review/approved
There was a problem hiding this comment.
Re-review of PR #3185 (feat(plugins): add publication policy hooks). The two previously-requested fixes are present and the new policy/runtime surface is sound. No blocking issues remain, but there are AGENTS.md comment-discipline violations in files touched by this PR.
Confirmed fixes:
- MCP
content_schedulenow requires_rev: the MCP tool-level input schema enforces it while the RESTPOST /_emdash/api/content/{collection}/{id}/schedulekeeps_revoptional for legacy callers. - Admin toolbar labels are localized via Lingui:
Shell.tsxwritestrendered strings tolocalStorage; the public fallback endpointGET /_emdash/api/visual-editing/toolbar-labelsreturns the standard{ data: { editMode, hideToolbar } }envelope and usesprivate, no-storecaching.
Other convention checks:
- UI: new/updated admin and toolbar code uses Lingui macros (
t,plural) and logical Tailwind utilities (me-auto,text-start, etc.);Dashboard.tsxwraps counts in<bdi dir="ltr">for RTL safety. - SQL: all prefix scans in
OptionsRepositoryuseescapeLike(...)andESSE '\\', and content/policy actions route through repositories with parameterized queries. - Authorization: content mutation routes gate actions via
requireOwnerPerm/content:publish_any; dismissal of scheduled rejections requirescontent:publish_any. - Query counts: the logged-out cold-start snapshot's single
name LIKE ...option query is a replacement, not an addition; hook resolution uses batchedIN (...)startup reads. - API envelope: the new public endpoint returns
{ data: ... }, not a naked value.
Remaining AGENTS.md violation: three files changed in this PR still contain prohibited issue/PR references (Discussion #1742) in comments. AGENTS.md explicitly forbids referencing issues, PRs, or review threads in code comments; that context belongs in commits/PR descriptions. I have line-anchored these below.
Findings
-
[needs fixing]
packages/admin/src/components/Shell.tsx:75-76AGENTS.md comment-discipline violation: this comment references
Discussion #1742. Issue/PR references are prohibited in code comments because they become stale narrative immediately after merge; that context belongs in the commit message or PR description. Describe the invariant directly instead.// Maintain the non-secret "an editor session may exist in this browser" // localStorage flag consumed by the public-site toolbar bootstrap // (`toolbar: "client"`). Set here — not in the login // flows — so every auth method (passkey, OAuth, magic link, dev bypass) // is covered. Opening the admin also un-dismisses the toolbar. // Key literals are duplicated in emdash core, which the admin can't import. -
[needs fixing]
packages/core/src/visual-editing/toolbar-bootstrap.ts:14AGENTS.md comment-discipline violation:
See Discussion #1742is an issue/PR reference and is not allowed in code comments. Replace it with an evergreen explanation of why only the editor flag is checked.* nothing else. Only the editor flag gates injection; missing localized * labels are fetched lazily rather than blocking the pill from mounting. -
[needs fixing]
packages/core/tests/unit/astro/request-context-toolbar-mode.test.ts:2AGENTS.md comment-discipline violation: the module comment references
Discussion #1742. Remove the issue reference./** * Tests for the `toolbar` config modes. * * - `"server"` (default): current behavior — the toolbar is injected * server-side for authenticated editors. * - `"client"`: public HTML is identical for everyone (bootstrap script,
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review of PR #3185. The three Discussion #1742 comment-discipline findings from the previous review are fixed in the files the prior review called out (Shell.tsx, toolbar-bootstrap.ts, request-context-toolbar-mode.test.ts).
The publication-policy hook design is sound and fits EmDash's architecture: capability-gated registration via hooks.content-policy:register, revision-fenced mutations, origin/actor-aware events for API/MCP/visual-editor/scheduler/system, scheduler rejection cleanup through the options table, and public-safe dashboard records. The changeset accurately describes the surface area.
Two new issues remain:
-
The dashboard dismissal endpoint is not wired up.
packages/core/src/astro/routes/api/admin/scheduled-policy-rejections/[collection]/[id].tsis implemented and unit-tested, butpackages/core/src/astro/integration/routes.tsnever callsinjectRoute()for it. Admin dashboard dismissal calls will 404, so the "blocked scheduled content" notice cannot be cleared through the UI. This defeats a headline feature of the PR and needs fixing before merge. -
Stale issue/PR references in a file changed by this PR.
packages/core/src/astro/middleware/request-context.tsstill contains three prohibited references (Discussion #1742× 2 and#1398× 1) in comments. AGENTS.md forbids these; the context belongs in commit messages/PR descriptions, not code comments.
Beyond these, the runtime wiring, MCP integration, visual-editing action-token renewal, SQL safety on prefix scans, authorization gating, and Lingui/RTL-safe admin UI look correct.
Findings
-
[needs fixing]
packages/core/src/astro/integration/routes.ts:119-122The new
DELETE /_emdash/api/admin/scheduled-policy-rejections/:collection/:idhandler exists and is tested, but it is never registered here. The admin dashboard calls this path to dismiss blocked scheduled-content notices; withoutinjectRoute()the endpoint returns 404 and the dismissal feature is broken.Add the route next to the related dashboard route.
injectRoute({ pattern: "/_emdash/api/dashboard", entrypoint: resolveRoute("api/dashboard.ts"), }); injectRoute({ pattern: "/_emdash/api/admin/scheduled-policy-rejections/[collection]/[id]", entrypoint: resolveRoute("api/admin/scheduled-policy-rejections/[collection]/[id].ts"), }); -
[needs fixing]
packages/core/src/astro/middleware/request-context.ts:13AGENTS.md comment-discipline violation: this comment references
Discussion #1742. Issue/PR references are prohibited in code comments because they become stale narrative immediately after merge; that context belongs in the commit message or PR description. Describe the invariant directly instead.* renders; the backend verifies the `_edit` param and redirects non-editors. -
[needs fixing]
packages/core/src/astro/middleware/request-context.ts:108AGENTS.md comment-discipline violation:
(#1398)is an issue/PR reference. Replace it with an evergreen explanation of why both cache layers must be disabled.// Both layers must be disabled: `Cache-Control` covers browsers/downstream // proxies; the route-cache opt-out covers the shared edge cache, which ignores // `Cache-Control`. -
[needs fixing]
packages/core/src/astro/middleware/request-context.ts:130AGENTS.md comment-discipline violation: this comment references
Discussion #1742. Remove the issue reference and state the behavior directly.* `?_edit` link degrades gracefully for everyone else: non-editors are redirected * to the canonical URL.
github-actions
Bot
added
review/approved
There was a problem hiding this comment.
Re-review of PR #3185. The publication-policy hook design remains sound: hooks.content-policy:register is capability-gated, native and sandboxed runners receive the new hooks, origin/actor metadata flows through the runtime for API/MCP/visual-editor/scheduler/system actions, scheduler rejections are persisted as bounded dashboard records with revision-fenced cleanup, and the admin UI correctly localizes the dismissal flow.
However, the two concrete findings from the previous review are still not fixed in this revision.
-
Dashboard dismissal route is still not wired up.
packages/core/src/astro/routes/api/admin/scheduled-policy-rejections/[collection]/[id].tsexists and has unit tests, butpackages/core/src/astro/integration/routes.tsnever callsinjectRoute()for it. The admin dashboard issuesDELETE /_emdash/api/admin/scheduled-policy-rejections/:collection/:idto dismiss blocked scheduled-content notices, so those calls will 404 and the dismissal feature will not work. -
Stale issue/PR references remain in
request-context.ts. Three comments still referenceDiscussion #1742and#1398, which violates AGENTS.md comment discipline: issue/PR references are stale narrative after merge; that context belongs in commit messages and PR descriptions.
Everything else checked — route authorization on the dismissal handler, dashboard stats SQL safety, visual-editing action-token/toolbar-labels wiring, Lingui server-label loading, RTL-safe admin markup, and the scheduler rejection path — looks correct. Please add the missing route and remove the three issue references, then this should be good to merge.
Findings
-
[needs fixing]
packages/core/src/astro/integration/routes.ts:119-122The new
DELETE /_emdash/api/admin/scheduled-policy-rejections/:collection/:idhandler exists and is unit-tested, but it is never registered here. The admin dashboard calls this path to dismiss blocked scheduled-content notices; withoutinjectRoute()the endpoint returns 404 and the dismissal feature is broken.Add the route next to the related dashboard route.
injectRoute({ pattern: "/_emdash/api/dashboard", entrypoint: resolveRoute("api/dashboard.ts"), }); injectRoute({ pattern: "/_emdash/api/admin/scheduled-policy-rejections/[collection]/[id]", entrypoint: resolveRoute("api/admin/scheduled-policy-rejections/[collection]/[id].ts"), }); -
[needs fixing]
packages/core/src/astro/middleware/request-context.ts:13AGENTS.md comment-discipline violation: this comment references
Discussion #1742. Issue/PR references are prohibited in code comments because they become stale narrative immediately after merge; that context belongs in the commit message or PR description. Describe the invariant directly instead.* renders; the backend verifies the `_edit` param and redirects non-editors. -
[needs fixing]
packages/core/src/astro/middleware/request-context.ts:108AGENTS.md comment-discipline violation:
(#1398)is an issue/PR reference. Replace it with an evergreen explanation of why both cache layers must be disabled.// Both layers must be disabled: `Cache-Control` covers browsers/downstream // proxies; the route-cache opt-out covers the shared edge cache, which ignores // `Cache-Control`. -
[needs fixing]
packages/core/src/astro/middleware/request-context.ts:130AGENTS.md comment-discipline violation: this comment references
Discussion #1742. Remove the issue reference and state the behavior directly.* `?_edit` link degrades gracefully for everyone else: non-editors are redirected * to the canonical URL.
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review of PR #3185. The publication-policy hook design remains the right change for the stated goal: hooks.content-policy:register is capability-gated, native and sandboxed runners enforce it, origin/actor metadata flows through API/MCP/visual-editor/scheduler paths, scheduler rejections are persisted as bounded dashboard records with revision-fenced cleanup, and the admin UI localizes and authorizes the dismissal flow.
Prior findings are fixed. The DELETE /_emdash/api/admin/scheduled-policy-rejections/:collection/:id route is now registered in packages/core/src/astro/integration/routes.ts, and the stale Discussion #1742 / #1398 issue references have been removed from request-context.ts.
What I checked: policy decision validation, hook registration/capability gating, runtime policy evaluation and revision fencing, scheduler rejection persistence and cleanup, dashboard handler SQL safety and authorization, visual-editing action-token renewal, route wiring, Lingui localization and RTL-safe admin markup, query-count snapshots, changeset prose, and documentation accuracy.
Headline conclusion: One remaining issue: the new public /_emdash/api/visual-editing/toolbar-labels route returns a raw { data: … } shape instead of the standard { success: true, data: … } API envelope. The client bootstrap still works because it reads body.data, but the endpoint is inconsistent with every other route and locks the shape in via its test. Convert it to apiSuccess() and update the route test.
github-actions
Bot
added
review/approved
github-actions
Bot
added
the
review/needs-rereview
There was a problem hiding this comment.
I reviewed the full diff for PR #3185 (publication-policy hooks, dashboard scheduler warnings, and visual-editing toolbar labels) plus the key changed files.
- Architecture & auth: Policy hooks run through the existing capability-gated hook pipeline; the new
hooks.content-policy:registercapability is documented. REST/MCP/visual-editor routes pass origin/actor so policy events can identify the source. The dismissal route checkscontent:publish_anyand uses compare-and-delete on the stored rejection revision. - Scheduler cursor:
publishDueContentuses a per-collection compare-and-set cursor, advances (even on publish failures) so the sweep drains the backlog, and compare-and-set with a stale revision prevents older concurrent sweeps from moving the cursor backwards. - Dashboard:
handleDashboardStatssafely queries by prefix with escapedLIKE, and the UI wires dismissal with query invalidation and conflict handling. - Visual-editing labels:
GET /_emdash/api/visual-editing/toolbar-labelsnow returns the standard{ success, data }apiSuccessenvelope, is listed as a public API route, and is used only for the client-mode bootstrap. Toolbar injection continues to setCache-Control: private, no-storeand opt out of the shared route cache. - Changeset: The
publication-policy-hooks.mdchangeset accurately describes the new capability, hook events, reason validation, rejection record lifecycle, and dashboard dismissal. - AGENTS.md conventions: No stale issue/PR references in new comments, no SQL interpolation, authorization uses RBAC permissions from
rbac.ts, and user-facing admin strings go through Lingui.
No blocking issues, regressions, or convention violations were found. The PR is clean.
3 participants
What does this PR do?
Adds capability-gated publication policy hooks for native and sandboxed plugins without granting content read, write, or lifecycle authority. Events identify every action origin and authenticated human actor.
Cancellation is validated and revision-fenced. Scheduler rejections become bounded dashboard records with revision-safe cleanup. Visual-editor actions use renewable editor-bound tokens.
Includes registry consent, both runners, runtime-backed tests, docs, authoring guidance, and a changeset. Builds on #3162.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm formathas been runAI-generated code disclosure
Screenshots / test output
Validated with root build/typecheck, lint, docs, focused tests, query counts, and adversarial review.