From b59cc453fe5de667cf2662e851eb2119e1e2a49a Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Thu, 20 Aug 2026 20:29:56 +0200 Subject: [PATCH 1/2] fix(#470): three core gaps the P5 acceptance run hit (C9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each was measured against a real core while installing the first extracted plugin, not inferred. Each failed in a way core could not see. #795 — `requires:` had no optionality, and two gates enforced it. C2b makes `ctx.services.get(name)` throw for a name in neither `requires:` nor `provides:`, so a plugin MUST declare anything it might resolve. The installer and the boot loop then treat every `requires:` entry as a hard prerequisite. A plugin with a degradable dependency was therefore unrepresentable: declaring it blocked the install, omitting it made resolution throw. New manifest field `optional_requires:`, same capability-ref syntax. It satisfies the declaration gate and neither enforcement gate — no `install.missing_capability`, no activation hold, no provider demanded. `ctx.services.getOptional(name)` is the accessor that says so at the call site; it is declaration-gated exactly like `get`, because a typo must not quietly become `undefined`. The ordering consequence is deliberate and documented rather than papered over: an optional dependency contributes no topo-sort edge, so an optional provider that IS installed may activate after its consumer. An edge from a link the kernel may not enforce would turn a mutual optional reference into a cycle — a boot failure caused by a dependency declared as skippable. The contract says to resolve optional services lazily instead. Surfaced on the install DTO (`Plugin.optional_requires`, and the registry teaser) so the consent UI can render these as optional rather than as blockers. `pluginServiceGrants.ts` loses the paragraph calling this an open design question — every remaining legacy-allowlist row now has a manifest fix available. #796 — the core migration ledger only ran when an LLM key was configured. `middleware/migrations/` is core's own directory, 47 files including C4's `plugin_public_path_grants` and C7's `plugin_sql_grants`. Its only production caller was inside harness-orchestrator's `activate()`, several hundred lines past an early return taken whenever no provider resolves. On a deployment with no key, core had no schema at all — not a degraded one, none — so recording either operator consent was structurally impossible. Silent by construction: nothing logged a migration error because no migration was ever attempted. `platform/coreMigrations.ts` now applies it at boot, before any tool plugin activates and independent of every provider. Ordering is load-bearing: `ToolPluginRuntime` reads a plugin's SQL-grant row while building its context, so the grant tables must exist by then. It opens its own short-lived connection from `DATABASE_URL` rather than waiting for `graphPool`, which the knowledge-graph plugin publishes during activation — waiting for it would reintroduce the same defect one layer up. The orchestrator's call is retained as an explicit second pass (one SELECT against a current ledger) and can never be the only caller again. #798 — no scoped plugin id could express a nav href. Core serves a plugin's bundled UI at `/p/:pluginId/ui/` and Express splits on a raw `/`, so `@scope/name` resolves only percent-encoded — measured encoded 200, raw 404. `HREF_SEGMENT` rejects `%` for a good reason: the shell decides "core destinations win" by string equality, which percent-encoding defeats. The only URL that worked was the only one the validator refused. `registerNav` now accepts `pluginUi: true` in place of a literal href and renders the canonical path itself from the id it already holds, so a plugin never hand-builds an encoded href. The literal-href validator is untouched — the tempting fix (widening `HREF_SEGMENT` to admit `%xx`, which the acceptance run patched locally) would weaken every literal href to fix one path core can spell for itself, and a test asserts it stays strict. web-ui derives the href from `pluginId` rather than validating the transmitted string. That is stronger than validation: the only encoded path the shell can emit is one it computed itself from a charset-checked id, so a version skew or a compromised control plane still cannot inject an arbitrary encoded path into the trusted header. It reuses `isValidPluginId` from C8b rather than adding a copy of the pattern. `uiRouteCatalog.ts` restates the manifest's plugin-id gate rather than importing it, because `manifestLoader.ts` keeps those declarations in the exact source form C8b's parity test anchors on. The restatement is pinned the same way — a test reads `manifestLoader.ts` and asserts both are character-identical, so drift fails a test rather than a nav entry. plugin-api 1.2.0 → 1.3.0 (MINOR, additive), CHANGELOG entry, golden snapshot regenerated deliberately: `getOptional`, `UiNavEntryInput.pluginUi`, `ResolvedUiNavEntry.pluginUi`, and `UiNavEntryInput.href` widened to optional. `UiNavEntry.href` stays required — the kernel resolves `pluginUi` to a concrete path at registration. Mutation-checked, five mutations each reddening the tests that claim the behaviour: dropping `optional_requires` from `declaredServiceNames` (4 fail), making the install-chain walk read optional entries (2), routing a `pluginUi` entry through `assertInAppHref` (3), making web-ui trust the transmitted href (2), and re-attaching the core ledger to `ANTHROPIC_API_KEY` (2). The pg test runs with every provider key stripped from the environment, so a regression reattaching the ledger to a credential fails here rather than in staging. Verified: middleware build, typecheck, typecheck:test (ratchet held at 406), lint (0 errors), 7880/7880 tests with pg; web-ui lint (0 errors, 47 pre-existing warnings), typecheck, 785/785 vitest, build. Core-decoupling ratchet 3300 → 3299. Two of that came from rewording `installServiceActivationTruthful.test.ts` (#799), which merged carrying two un-baselined dev-platform references — main measured 3302 against its own 3300 baseline before this branch. Reworded rather than raised. Fixes #795, #796, #798 --- .../harness-orchestrator/src/plugin.ts | 11 + middleware/packages/plugin-api/CHANGELOG.md | 49 ++ .../api-snapshot/plugin-api.d.ts.snap | 8 +- middleware/packages/plugin-api/package.json | 2 +- .../packages/plugin-api/src/pluginContext.ts | 81 +++- middleware/src/api/admin-v1.ts | 13 + middleware/src/api/registry-v1.ts | 4 + middleware/src/index.ts | 25 ++ middleware/src/platform/coreMigrations.ts | 100 +++++ middleware/src/platform/pluginContext.ts | 11 + .../src/platform/pluginServiceGrants.ts | 26 +- middleware/src/platform/uiRouteCatalog.ts | 105 ++++- .../src/plugins/builder/previewRuntime.ts | 13 + middleware/src/plugins/capabilityResolver.ts | 19 + middleware/src/plugins/manifestLoader.ts | 29 +- middleware/src/routes/store.ts | 7 + middleware/test/coreMigrations.pg.test.ts | 216 +++++++++ .../installServiceActivationTruthful.test.ts | 5 +- middleware/test/optionalRequires.test.ts | 423 ++++++++++++++++++ .../test/uiRouteCatalogPluginUiNav.test.ts | 222 +++++++++ .../decoupling-baseline.json | 6 +- web-ui/app/_lib/__tests__/navParse.test.ts | 79 ++++ web-ui/app/_lib/navigation.ts | 51 ++- 23 files changed, 1470 insertions(+), 35 deletions(-) create mode 100644 middleware/src/platform/coreMigrations.ts create mode 100644 middleware/test/coreMigrations.pg.test.ts create mode 100644 middleware/test/optionalRequires.test.ts create mode 100644 middleware/test/uiRouteCatalogPluginUiNav.test.ts diff --git a/middleware/packages/harness-orchestrator/src/plugin.ts b/middleware/packages/harness-orchestrator/src/plugin.ts index c165bd35..e64f0b6b 100644 --- a/middleware/packages/harness-orchestrator/src/plugin.ts +++ b/middleware/packages/harness-orchestrator/src/plugin.ts @@ -1095,6 +1095,17 @@ export async function activate( let reloadBus: ReloadBus | undefined; if (graphPool) { try { + // #796 — SECOND PASS, not the owner. `middleware/migrations/` is core's + // directory and core now applies it at boot, before any plugin + // activates and regardless of whether a provider key exists. This call + // used to be its only production trigger, which made core's schema a + // side effect of an LLM key being configured. + // + // It stays because it costs one SELECT against a current ledger (the + // migrator takes no lock when nothing is pending) and it keeps this + // plugin working when it is activated outside core's boot path — an + // embedding host, a fixture harness. It must never be the only caller + // again. await runMultiOrchestratorMigrations(graphPool, (m) => ctx.log(`[harness-orchestrator] ${m}`), ); diff --git a/middleware/packages/plugin-api/CHANGELOG.md b/middleware/packages/plugin-api/CHANGELOG.md index a86ecaa7..fd8b48de 100644 --- a/middleware/packages/plugin-api/CHANGELOG.md +++ b/middleware/packages/plugin-api/CHANGELOG.md @@ -8,6 +8,55 @@ Versioning is SemVer over the **exported type surface**. Removing or narrowing an exported type, or adding a required member to an interface a plugin implements, is a major. +## 1.3.0 — 2026-08-20 + +Additive. Two shapes a plugin could not express before, both found by the +epic #470 P5 acceptance run against a real core: a dependency it can survive +the absence of, and a nav entry pointing at its own bundled UI when its id is +scoped. Every existing consumer keeps compiling — the new members are optional +or additive, and no existing member changed meaning. + +### Added + +- **`ctx.services.getOptional(name)`** (`(name: string) => T | undefined`) + — the accessor for a capability declared under the new manifest field + `optional_requires:` (#795). Declaration-gated exactly like `get`, so an + undeclared name still throws `ServiceNotDeclaredError` and a typo cannot + quietly become `undefined`; what it adds is a call site that says absence is + survivable. `optional_requires:` entries use the same capability-ref syntax + as `requires:` and satisfy the same declaration gate, but are NOT an + activation dependency: the installer raises no + `install.missing_capability`, and the capability resolver neither demands + nor orders a provider for them. + + The ordering consequence is part of the contract: with no activation edge, + an optional provider that IS installed may activate after its consumer. + Resolve optional services lazily, at first use, rather than caching the + result of one call during `activate()`. + +- **`UiNavEntryInput.pluginUi`** (`true | undefined`) — ask the kernel to + render the canonical path to this plugin's own bundled UI instead of + supplying a literal `href` (#798). A scoped plugin id resolves only + percent-encoded (`/plugin-ui/%40acme%2Fwidget`), and percent-encoding is + precisely what the literal-`href` validator refuses — so a scoped plugin + previously had no spelling that both validated and worked. Supply exactly + one of `href` or `pluginUi: true`; supplying both, or neither, throws. + +- **`ResolvedUiNavEntry.pluginUi`** (`true | undefined`) — set on entries + registered that way, so the web UI re-derives the href from `pluginId` + locally instead of trusting a percent-encoded string across a deployment + boundary. + +### Changed + +- **`UiNavEntryInput.href`** is now optional (`string | undefined`), because + a `pluginUi: true` entry supplies none. Widening an input field: every + plugin that passes an `href` today is unaffected. +- **`UiNavEntry.href`** stays required — the kernel resolves `pluginUi` to a + concrete path at registration, so a catalogued entry always has one. +- **`ServiceNotDeclaredError`**'s message now names `optional_requires:` as a + fix alongside `requires:` and `provides:`. + ## 1.2.0 — 2026-08-20 Additive. A plugin may now be handed a Postgres pool and own tables in the diff --git a/middleware/packages/plugin-api/api-snapshot/plugin-api.d.ts.snap b/middleware/packages/plugin-api/api-snapshot/plugin-api.d.ts.snap index f639fc18..9110204d 100644 --- a/middleware/packages/plugin-api/api-snapshot/plugin-api.d.ts.snap +++ b/middleware/packages/plugin-api/api-snapshot/plugin-api.d.ts.snap @@ -1474,6 +1474,7 @@ constructor(pluginId: string, capability: string); } export interface ServicesAccessor { get(name: string): T | undefined; +getOptional(name: string): T | undefined; has(name: string): boolean; provide(name: string, impl: T | PerCallerFactory): () => void; replace(name: string, impl: T | PerCallerFactory): () => void; @@ -1568,13 +1569,15 @@ readonly pluginId: string; } export interface UiNavEntryInput { readonly navId: string; -readonly href: string; +readonly href?: string; +readonly pluginUi?: true; readonly cluster?: string; readonly order?: number; readonly label: Readonly>; } -export interface UiNavEntry extends UiNavEntryInput { +export interface UiNavEntry extends Omit { readonly pluginId: string; +readonly href: string; } export interface ResolvedUiNavEntry { readonly pluginId: string; @@ -1583,6 +1586,7 @@ readonly href: string; readonly cluster?: string; readonly order: number; readonly label: string; +readonly pluginUi?: true; } export interface NotificationsAccessor { send(payload: NotificationPayload): Promise; diff --git a/middleware/packages/plugin-api/package.json b/middleware/packages/plugin-api/package.json index ccd6e695..ac375623 100644 --- a/middleware/packages/plugin-api/package.json +++ b/middleware/packages/plugin-api/package.json @@ -1,6 +1,6 @@ { "name": "@omadia/plugin-api", - "version": "1.2.0", + "version": "1.3.0", "private": true, "type": "module", "main": "dist/index.js", diff --git a/middleware/packages/plugin-api/src/pluginContext.ts b/middleware/packages/plugin-api/src/pluginContext.ts index d44d9c2a..d3047e0f 100644 --- a/middleware/packages/plugin-api/src/pluginContext.ts +++ b/middleware/packages/plugin-api/src/pluginContext.ts @@ -535,7 +535,8 @@ export class ServiceNotDeclaredError extends Error { constructor(pluginId: string, capability: string) { super( `plugin '${pluginId}' called ctx.services.get('${capability}') but its manifest does not declare that capability — ` + - `add '${capability}@' to the manifest's \`requires:\` list (or \`provides:\` if this plugin is the provider)`, + `add '${capability}@' to the manifest's \`requires:\` list (or \`optional_requires:\` when absence is survivable, ` + + `or \`provides:\` if this plugin is the provider)`, ); this.name = 'ServiceNotDeclaredError'; this.pluginId = pluginId; @@ -580,6 +581,31 @@ export interface ServicesAccessor { * not declare `name` — that is a manifest bug, not a missing provider, and * the two must not be reported the same way. */ get(name: string): T | undefined; + /** + * Resolve a capability the plugin declared as OPTIONAL + * (`optional_requires:` in the manifest), where "no provider installed" + * is a supported steady state rather than a misconfiguration. + * + * Declaration-gated on exactly the same terms as {@link get}: a name in + * neither `requires:`, `optional_requires:` nor `provides:` throws + * {@link ServiceNotDeclaredError}, because a typo must not silently + * become `undefined`. + * + * The difference from {@link get} is the contract it advertises, not the + * lookup. `get` is paired with `requires:`, which the installer and the + * boot loop both treat as a hard prerequisite — so a `get` that returns + * `undefined` normally means something upstream failed. `getOptional` is + * paired with `optional_requires:`, which neither gate enforces, so + * `undefined` here is an expected answer and the caller is expected to + * carry a degradation path for it. + * + * Note the ordering caveat that comes with optionality: an optional + * dependency contributes no activation edge, so a provider that IS + * installed may not have activated yet when the consumer's `activate()` + * runs. Resolve optional services lazily (at first use) rather than + * caching the result of a single call during activation. + */ + getOptional(name: string): T | undefined; /** Whether a provider is currently registered. Ungated — see the interface * doc. */ has(name: string): boolean; @@ -1001,6 +1027,12 @@ export interface UiRoutesAccessor { * web-ui pages (a built-in package) has a nav entry and no uiRoute; * a plugin serving its own HTML has both. * + * Supply either a literal `href` (validated as a canonical in-app path) + * or `pluginUi: true`, which asks the kernel to render the canonical + * path to this plugin's own bundled UI — the only way a scoped plugin + * id can express a nav destination, since that path must be + * percent-encoded and a literal href may not be. + * * Returns a dispose handle the plugin MUST call from its `close()`. * The kernel additionally drops every entry by source on deactivate, * so a leaked handle cannot outlive the plugin. @@ -1044,11 +1076,32 @@ export interface UiNavEntryInput { /** Stable id within the plugin. Combined with pluginId as the key. */ readonly navId: string; /** - * Absolute in-app path (e.g. `/admin/dev-platform`). Must start with - * exactly one `/` — protocol-relative (`//host`) and scheme-bearing - * values are rejected so a manifest cannot point the nav off-origin. + * Absolute in-app path (e.g. `/admin/reports`). Must start with exactly + * one `/` — protocol-relative (`//host`) and scheme-bearing values are + * rejected so a manifest cannot point the nav off-origin. Segments are + * confined to the RFC 3986 unreserved set: no query, no fragment, no + * percent-encoding, no dot-segments. + * + * Mutually exclusive with {@link pluginUi}; exactly one of the two must + * be supplied. */ - readonly href: string; + readonly href?: string; + /** + * Point the entry at THIS plugin's own bundled UI instead of a literal + * path, and let the kernel spell the URL. + * + * A plugin that ships a compiled SPA is served at `/p//ui/` + * and embedded by the shell's host page at `/plugin-ui/`. For + * a scoped id like `@acme/widget` the only URL that resolves is the + * percent-encoded one (`%40acme%2Fwidget`) — and percent-encoding is + * exactly what the `href` validator refuses, deliberately, because a + * literal href has to be comparable to a core path by string equality. + * + * So the plugin states the intent and the kernel renders the canonical + * encoded path from the id it already knows. A plugin never hand-builds + * an encoded href, and the literal-href rule stays strict. + */ + readonly pluginUi?: true; /** * Optional cluster to nest under (e.g. `adminCluster`). Rendered as a * top-level entry when omitted, or when the shell has no cluster by @@ -1064,9 +1117,15 @@ export interface UiNavEntryInput { readonly label: Readonly>; } -/** Catalogue-resolved nav entry — pluginId injected by the kernel. */ -export interface UiNavEntry extends UiNavEntryInput { +/** + * Catalogue-resolved nav entry — `pluginId` injected by the kernel, and + * `href` no longer optional: a `pluginUi: true` input is resolved to the + * canonical host-page path at registration, so every stored entry carries + * a concrete destination. + */ +export interface UiNavEntry extends Omit { readonly pluginId: string; + readonly href: string; } /** @@ -1081,6 +1140,14 @@ export interface ResolvedUiNavEntry { readonly cluster?: string; readonly order: number; readonly label: string; + /** + * Present iff the entry was registered with `pluginUi: true`. The shell + * uses it to re-derive `href` from `pluginId` locally instead of + * trusting the transmitted string — the middleware is a separate + * deployable, and a percent-encoded href is the one shape the shell's + * own defensive href rule cannot check character by character. + */ + readonly pluginUi?: true; } /** diff --git a/middleware/src/api/admin-v1.ts b/middleware/src/api/admin-v1.ts index e2d73b72..a98e1380 100644 --- a/middleware/src/api/admin-v1.ts +++ b/middleware/src/api/admin-v1.ts @@ -514,6 +514,19 @@ export interface Plugin { * to empty array). The kernel rejects boot if any `requires` has no * matching `provides` across the installed plugin set. */ requires: string[]; + /** + * Capabilities this plugin may use but can run without (#795). Same + * capability-ref syntax as {@link requires}, and the same effect on the + * `ctx.services` declaration gate — but NOT an activation dependency: + * the installer does not refuse the install when nothing provides one, + * the capability resolver neither orders nor demands a provider, and + * `ctx.services.getOptional(name)` simply answers `undefined`. + * + * Absent when the manifest declares none; read it as `?? []`. Surfaced + * on the install DTO so the consent UI can render these prerequisites + * as optional rather than as blockers. + */ + optional_requires?: string[]; /** * Builder service-type declarations (OB — service-type auto-discovery). * Integration plugins list every `ctx.services.provide(...)` surface they diff --git a/middleware/src/api/registry-v1.ts b/middleware/src/api/registry-v1.ts index 552f6282..715cdef9 100644 --- a/middleware/src/api/registry-v1.ts +++ b/middleware/src/api/registry-v1.ts @@ -44,6 +44,10 @@ export interface RegistryVersionEntry { export interface RegistryManifestSummary { provides?: string[]; requires?: string[]; + /** Capabilities the plugin can run without (#795). Same capability-ref + * syntax as `requires`, but never a reason to refuse an install — the + * consent UI renders these as optional prerequisites. */ + optional_requires?: string[]; depends_on?: string[]; /** Setup fields the operator must fill at install-time. Mirrors * `PluginSetupField` but kept loose here to avoid a hard schema coupling. */ diff --git a/middleware/src/index.ts b/middleware/src/index.ts index 91a7f769..8143e0a4 100644 --- a/middleware/src/index.ts +++ b/middleware/src/index.ts @@ -229,6 +229,7 @@ import { RefreshStore } from './auth/refreshStore.js'; import { EmailWhitelist } from './auth/whitelist.js'; import { resolveSessionSigningKey } from './auth/sessionSigningKey.js'; import { runAuthMigrations } from './auth/migrator.js'; +import { runCoreMigrations } from './platform/coreMigrations.js'; import { runProfileStorageMigrations } from './profileStorage/migrator.js'; import { LiveProfileStorageService } from './profileStorage/liveProfileStorageService.js'; import { runProfileSnapshotMigrations } from './profileSnapshots/migrator.js'; @@ -1699,6 +1700,30 @@ async function main(): Promise { const ms365IntegrationId = 'de.byte5.integration.microsoft365'; const calendarAgentId = 'de.byte5.agent.calendar'; + // #796 (epic #470 C9 / G3) — core's own base schema, applied by core, + // BEFORE any plugin activates and independent of every provider key. + // + // This used to be a side effect of the harness-orchestrator plugin + // activating, which returns early when no LLM provider is configured — so a + // deployment without an Anthropic key had no `_multi_orchestrator_migrations` + // ledger, no `plugin_public_path_grants` and no `plugin_sql_grants`, and + // therefore no way to record either operator consent. Nothing logged it, + // because no migration was ever attempted. + // + // Ordering is load-bearing, not tidiness: `ToolPluginRuntime` reads a plugin's + // SQL-grant row while building its context, so the grant tables have to exist + // by the time the line below runs. See `platform/coreMigrations.ts` for why it + // opens its own connection instead of waiting for `graphPool`. + const coreMigrations = await runCoreMigrations({ + databaseUrl: process.env['DATABASE_URL'], + log: (m) => { console.log(m); }, + }); + console.log( + coreMigrations === 'no-database' + ? '[middleware] core migrations SKIPPED — no DATABASE_URL (in-memory backend)' + : '[middleware] core migrations applied (middleware/migrations)', + ); + // Activate tool / extension / integration plugins FIRST. Their // activate() populates nativeToolRegistry + pluginRouteRegistry + // serviceRegistry (incl. the MemoryStore provided by @omadia/memory diff --git a/middleware/src/platform/coreMigrations.ts b/middleware/src/platform/coreMigrations.ts new file mode 100644 index 00000000..cfedf6e5 --- /dev/null +++ b/middleware/src/platform/coreMigrations.ts @@ -0,0 +1,100 @@ +import { Pool } from 'pg'; + +import { runMultiOrchestratorMigrations } from '@omadia/orchestrator'; + +/** + * Core's own base schema, applied by core (#796, epic #470 C9 / G3). + * + * WHAT WAS WRONG + * -------------- + * `middleware/migrations/` is a core-owned directory — 47 files, including + * `0046_plugin_public_path_grants.sql` (C4's operator-consent table for + * plugin public paths) and `0047_plugin_sql_grants.sql` (C7's operator-consent + * table for plugin SQL access). Its only production caller was the + * harness-orchestrator plugin's `activate()`, several hundred lines after an + * early return: + * + * const provider = await resolveLlmProvider(...); + * if (!provider) return { async close() {...} }; // <- ledger never runs + * + * So on a deployment with no LLM provider key, core had no schema. Not a + * degraded one — none. `_multi_orchestrator_migrations` did not exist, + * neither grant table existed, and recording either consent was structurally + * impossible. The failure was silent by construction: nothing logged a + * migration error because no migration was ever attempted. + * + * THE RULE + * -------- + * Core's schema is core's responsibility, and it cannot be conditional on a + * plugin choosing to activate, let alone on a credential unrelated to it. + * This runs at boot, before any tool plugin activates, whatever the provider + * configuration is. + * + * WHY ITS OWN POOL + * ---------------- + * `graphPool` is published into the service registry by the knowledge-graph + * plugin during `activateAllInstalled()` — i.e. after the point where these + * tables must already exist, since the SQL-grant gate reads a grant row while + * building each plugin's context. Waiting for that pool would reintroduce the + * same defect one layer up: core's schema depending on a plugin. So core opens + * a small, short-lived connection of its own from `DATABASE_URL`, applies the + * ledger, and closes it. Two connections for a few hundred milliseconds at + * boot is the entire cost. + * + * IDEMPOTENCE + * ----------- + * `runMultiOrchestratorMigrations` reads its ledger first and takes the + * `_multi_orchestrator_migrations` advisory lock only when there is work owed, + * so the steady-state boot is one SELECT and the orchestrator's own later call + * is a no-op second pass. Multiple replicas booting together serialise on the + * same lock they always did. + */ + +/** Outcome of a boot-time core-migration run. Returned rather than logged-only + * so callers (and tests) can assert which branch was taken. */ +export type CoreMigrationsOutcome = + /** No `DATABASE_URL` — the in-memory backend is in use and there is no + * database to migrate. Not an error: tests and zero-config dev boot here. */ + | 'no-database' + /** The ledger is current, whether this call applied files or found none. */ + | 'applied'; + +export interface CoreMigrationsOptions { + /** Postgres connection string. Omit / leave empty to skip. */ + readonly databaseUrl?: string | undefined; + /** Where progress goes. Defaults to a no-op so tests stay quiet. */ + readonly log?: ((msg: string) => void) | undefined; + /** + * Pool factory seam. Defaults to a real `pg.Pool`; overridden in tests that + * want to hand in a pool against a scratch database without going through + * the environment. + */ + readonly createPool?: ((connectionString: string) => Pool) | undefined; +} + +export async function runCoreMigrations( + opts: CoreMigrationsOptions = {}, +): Promise { + const databaseUrl = opts.databaseUrl?.trim(); + const log = opts.log ?? ((): void => undefined); + if (!databaseUrl) return 'no-database'; + + const createPool = + opts.createPool ?? + ((connectionString: string): Pool => + // Two connections is enough: the migrator uses exactly one, and the + // spare keeps a transient checkout failure from stalling boot. The pool + // is closed before boot continues, so it never competes with the + // long-lived pools plugins open later. + new Pool({ connectionString, max: 2, idleTimeoutMillis: 1_000 })); + + const pool = createPool(databaseUrl); + try { + await runMultiOrchestratorMigrations(pool, log); + return 'applied'; + } finally { + // `end()` must not mask a migration failure, and must not itself fail the + // boot: the migrations are already committed by the time we get here. + await pool.end().catch(() => undefined); + } +} diff --git a/middleware/src/platform/pluginContext.ts b/middleware/src/platform/pluginContext.ts index a5165176..e0c397f8 100644 --- a/middleware/src/platform/pluginContext.ts +++ b/middleware/src/platform/pluginContext.ts @@ -326,6 +326,17 @@ export function createPluginContext( ? (borrowPool(resolved as Pool, agentId) as T) : resolved; }, + // #795 — the accessor an `optional_requires:` entry is consumed + // through. Same gates in the same order as `get`: a name the manifest + // declares nowhere is still a manifest bug and still throws, because a + // typo that quietly became `undefined` is precisely the failure the + // declaration gate exists to prevent. What differs is the contract the + // caller signs up to — `undefined` is an answer here, not a symptom — + // and the fact that the kernel never held the plugin's activation back + // waiting for a provider. + getOptional(name: string): T | undefined { + return services.get(name); + }, has(name: string): boolean { return serviceRegistry.has(name); }, diff --git a/middleware/src/platform/pluginServiceGrants.ts b/middleware/src/platform/pluginServiceGrants.ts index 4a58f230..57b0c053 100644 --- a/middleware/src/platform/pluginServiceGrants.ts +++ b/middleware/src/platform/pluginServiceGrants.ts @@ -23,6 +23,9 @@ * invented here: * * - `requires: ["knowledgeGraph@^1"]` grants `get('knowledgeGraph')`. + * - `optional_requires: ["turnContext@1"]` grants `get('turnContext')` and + * `getOptional('turnContext')` without making the capability an + * activation prerequisite (#795). * - `provides: ["memoryStore@1"]` grants `get('memoryStore')` — a plugin * may always read back its own registration; it holds the implementation * anyway, so this is not an escalation. @@ -59,13 +62,14 @@ * * RETIRING IT * ----------- - * Each row is retired by adding the capability to that plugin's manifest — but - * note `requires:` is also the *activation* dependency (`resolveEligiblePlugins` - * holds back a consumer whose requires are unmet), so a plugin that consumes a - * service *optionally* cannot express that today. Declaring it would make an - * optional dependency mandatory and could stop the plugin activating. That - * missing "optional requires" expression is the open design question this - * allowlist defers, not a shortcut around work that is already possible. + * Each row is retired by adding the capability to that plugin's manifest. Until + * #795 that was not always possible: `requires:` is also the *activation* + * dependency (`resolveEligiblePlugins` holds back a consumer whose requires are + * unmet), so declaring an optionally-consumed service would have made it + * mandatory and could have stopped the plugin activating. `optional_requires:` + * now expresses exactly that case — it grants the same declaration this gate + * asks for and creates no activation prerequisite — so every remaining row here + * has a manifest fix available and the allowlist can be drained. */ import { @@ -203,6 +207,14 @@ export function declaredServiceNames( const names = new Set(); for (const raw of [ ...(entry.plugin.requires ?? []), + // #795 — an optional dependency is still a DECLARATION. It says "I may + // resolve this", which is exactly the question this gate asks; what it + // does not say is "hold my activation until someone provides it", which + // is a different gate (capabilityResolver) and stays untouched. Without + // this line the two gates contradict each other: C2b would demand the + // capability be listed, and listing it under `requires:` would make a + // degradable dependency mandatory. + ...(entry.plugin.optional_requires ?? []), ...(entry.plugin.provides ?? []), ]) { try { diff --git a/middleware/src/platform/uiRouteCatalog.ts b/middleware/src/platform/uiRouteCatalog.ts index 9c548a6a..9d2e732a 100644 --- a/middleware/src/platform/uiRouteCatalog.ts +++ b/middleware/src/platform/uiRouteCatalog.ts @@ -30,9 +30,59 @@ const LOCALE_CODE = /^[a-z]{2}(?:-[A-Za-z0-9]+)*$/; /** * Characters permitted in a single href path segment — the RFC 3986 * "unreserved" set. Deliberately excludes `%`, `?`, `#`, and `\`. + * + * This stays strict (#798). The rule exists because the shell decides + * "core destinations win" by comparing hrefs for string equality, and + * percent-encoding breaks that comparison — so widening it to admit + * `%xx` would weaken every literal href to fix one path that core can + * spell for itself. See {@link pluginUiHref}. */ const HREF_SEGMENT = /^[A-Za-z0-9\-._~]+$/; +/** + * Plugin ids the kernel will encode into a nav href. Mirrors + * `PLUGIN_ID_PATTERN` in `plugins/manifestLoader.ts` — npm-style, lowercase, + * optionally `@scope/`-prefixed. + * + * Re-stated rather than imported, matching what web-ui's `_lib/pluginId.ts` + * does for the same pattern (C8b): `manifestLoader.ts` keeps these two + * declarations in a form a source-reading parity test can anchor on, and + * exporting them would reformat the lines that test matches. So the + * restatement is PINNED the same way instead — + * `uiRouteCatalogPluginUiNav.test.ts` reads `manifestLoader.ts` and asserts + * both are character-identical, which turns this comment into a check. + * + * Checked rather than assumed for a second reason: `registerNav` is called + * with a kernel-supplied id, but an id that never passed the manifest gate + * (a hand-built harness, a future programmatic registration) must not be + * percent-encoded into the shell's chrome unexamined. + */ +const ENCODABLE_PLUGIN_ID = + /^(?:@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/; +const PLUGIN_ID_MAX_LENGTH = 214; + +/** + * The canonical in-app path to a plugin's own bundled UI. + * + * Core serves the compiled bundle at `/p//ui/` and the web UI + * hosts it in an iframe at `/plugin-ui/`. For a scoped id, the + * encoded spelling is the ONLY one that resolves — Express and the Next + * router both split on a raw `/`, so `@acme/widget` becomes two segments + * and matches nothing. Measured on a live core: encoded 200, raw 404. + * + * That spelling is also, by construction, the one {@link HREF_SEGMENT} + * refuses. Both rules are right; what was missing was somewhere for them + * to meet. This function is that place: the plugin declares + * `pluginUi: true`, the kernel renders the path from the id it already + * holds, and no plugin ever hand-builds a percent-encoded href. + * + * Exported so the web UI's mirrored derivation and this module's tests can + * assert against one definition instead of two string literals. + */ +export function pluginUiHref(pluginId: string): string { + return `/plugin-ui/${encodeURIComponent(pluginId)}`; +} + /** * Reject control characters and bidirectional-formatting codepoints. * @@ -289,6 +339,11 @@ export class UiRouteCatalog { * in-app paths and labels are length- and charset-checked, because * both are rendered inside the shell's own header where an operator * has every reason to trust what they see. + * + * The destination arrives one of two ways (#798): a literal `href`, + * validated as a canonical in-app path, or `pluginUi: true`, which asks + * the kernel to render {@link pluginUiHref} for this plugin. Exactly one + * of the two must be supplied. */ registerNav(pluginId: string, input: UiNavEntryInput): () => void { if (typeof pluginId !== 'string' || pluginId.length === 0) { @@ -306,7 +361,49 @@ export class UiRouteCatalog { ); } const context = `UiRouteCatalog.registerNav(${pluginId}/${input.navId})`; - assertInAppHref(context, input.href); + + // #798 — two ways to name a destination, never both. Accepting both + // would leave the entry's real target ambiguous at exactly the moment + // it matters (a plugin sending a literal href AND `pluginUi: true` is + // asking two different questions), so the ambiguity is refused here + // rather than resolved by precedence. + const wantsPluginUi = input.pluginUi === true; + if (input.pluginUi !== undefined && !wantsPluginUi) { + throw new Error( + `${context}: pluginUi must be literal \`true\` when present`, + ); + } + if (wantsPluginUi && input.href !== undefined) { + throw new Error( + `${context}: supply either 'href' or 'pluginUi: true', not both — 'pluginUi' means the kernel renders the href`, + ); + } + if (!wantsPluginUi && input.href === undefined) { + throw new Error( + `${context}: a nav entry needs a destination — supply 'href', or 'pluginUi: true' to point at this plugin's own bundled UI`, + ); + } + + let href: string; + if (wantsPluginUi) { + if ( + pluginId.length > PLUGIN_ID_MAX_LENGTH || + !ENCODABLE_PLUGIN_ID.test(pluginId) + ) { + throw new Error( + `${context}: pluginUi requires an npm-style plugin id (got '${pluginId}') — the kernel will not percent-encode an unrecognised id into the shell's chrome`, + ); + } + // Deliberately NOT run through `assertInAppHref`: this href is + // percent-encoded and would fail it. The safety the validator buys + // for untrusted input is bought differently here — the string is + // built by core from a charset-checked id, so there is no untrusted + // portion left to validate. + href = pluginUiHref(pluginId); + } else { + assertInAppHref(context, input.href); + href = input.href; + } if ( input.cluster !== undefined && (input.cluster.length > MAX_CLUSTER_LENGTH || @@ -337,8 +434,9 @@ export class UiRouteCatalog { const entry: UiNavEntry = { pluginId, navId: input.navId, - href: input.href, + href, label, + ...(wantsPluginUi ? { pluginUi: true as const } : {}), ...(input.cluster !== undefined ? { cluster: input.cluster } : {}), ...(input.order !== undefined ? { order: input.order } : {}), }; @@ -384,6 +482,9 @@ export class UiRouteCatalog { href: entry.href, order, label: resolveLabel(entry.label, locale), + // Carried to the shell so it can re-derive the href from + // `pluginId` itself rather than trusting this string (#798). + ...(entry.pluginUi === true ? { pluginUi: true as const } : {}), ...(entry.cluster !== undefined ? { cluster: entry.cluster } : {}), }; }) diff --git a/middleware/src/plugins/builder/previewRuntime.ts b/middleware/src/plugins/builder/previewRuntime.ts index cba305e0..fd26637b 100644 --- a/middleware/src/plugins/builder/previewRuntime.ts +++ b/middleware/src/plugins/builder/previewRuntime.ts @@ -221,6 +221,11 @@ export interface PreviewPluginContext { * `docs/harness-platform/HANDOFF-2026-05-04-preview-services-undefined.md`. */ readonly services: { get(name: string): T | undefined; + /** #795 — the optional-dependency accessor. Ungated in preview (there is + * no installed manifest to check a declaration against), so it resolves + * identically to `get`; what a previewed agent depends on is the return + * value, and that is the same. */ + getOptional(name: string): T | undefined; has(name: string): boolean; provide(name: string, impl: T): () => void; replace(name: string, impl: T): () => void; @@ -932,6 +937,14 @@ function createStubContext(opts: { if (localServices.has(name)) return localServices.get(name) as T; return host ? host.get(name) : undefined; }, + // #795 — the preview runtime is ungated by design (there is no + // installed manifest to check against), so optional and required + // resolution take the same path here. The distinction that matters + // to a previewed agent is the return value, and that is identical. + getOptional: (name: string): T | undefined => { + if (localServices.has(name)) return localServices.get(name) as T; + return host ? host.get(name) : undefined; + }, has: (name: string): boolean => localServices.has(name) || (host ? host.has(name) : false), provide: (name: string, impl: T): (() => void) => { diff --git a/middleware/src/plugins/capabilityResolver.ts b/middleware/src/plugins/capabilityResolver.ts index 028d36c9..35713b1c 100644 --- a/middleware/src/plugins/capabilityResolver.ts +++ b/middleware/src/plugins/capabilityResolver.ts @@ -101,6 +101,20 @@ export function resolveCapabilities( for (const consumerId of eligibleIds) { const consumer = catalog.get(consumerId); + // `requires` ONLY. `optional_requires` (#795) is deliberately not read + // here, and the omission is the whole feature: an optional capability + // must neither hold a consumer back nor contribute an ordering edge. + // + // Skipping the edge is the part worth arguing about, because an + // optional provider that IS installed may then activate after its + // consumer. That is accepted knowingly: optional dependencies are the + // shape most likely to point back at the plugin that offers them + // (A optionally uses B, B requires A), and an edge from a link the + // kernel is not allowed to enforce would turn that into a topo-sort + // cycle — a boot failure caused by a dependency declared as skippable. + // The cost lands in the API contract instead, where it is visible: + // `ctx.services.getOptional` documents that optional services must be + // resolved lazily rather than cached during activate(). const requires = consumer?.plugin.requires ?? []; const consumerUnresolved: string[] = []; @@ -314,6 +328,11 @@ export function walkCapabilityInstallChain( const walk = (pluginId: string, depth: number): void => { const entry = catalog.get(pluginId); if (!entry) return; + // `requires` only — an `optional_requires` entry (#795) is never a + // reason to refuse an install, so it must not enter the chain. Nor is + // its would-be provider walked: pulling an optional provider's own + // unmet requires into the 409 would block the install on a plugin the + // operator never asked for. for (const rawReq of entry.plugin.requires) { let capRef: CapabilityRef; try { diff --git a/middleware/src/plugins/manifestLoader.ts b/middleware/src/plugins/manifestLoader.ts index 460410a0..c72d5cd0 100644 --- a/middleware/src/plugins/manifestLoader.ts +++ b/middleware/src/plugins/manifestLoader.ts @@ -358,6 +358,11 @@ export function adaptManifestV1(doc: Record): Plugin | null { const jobs = extractJobs(doc['jobs']); const provides = extractCapabilityList(doc['provides'], id, 'provides'); const requires = extractCapabilityList(doc['requires'], id, 'requires'); + const optionalRequires = extractCapabilityList( + doc['optional_requires'], + id, + 'optional_requires', + ); const serviceTypes = extractServiceTypes(doc['service_types'], id); const channel = kind === 'channel' ? extractChannelBlock(doc['channel']) : undefined; @@ -465,6 +470,14 @@ export function adaptManifestV1(doc: Record): Plugin | null { privacy_class: privacyClass, }; let result: Plugin = base; + // Only attached when the manifest actually declares one, mirroring + // `service_types` below: `undefined` and `[]` mean the same thing to every + // consumer (all of which read it as `?? []`), and omitting the key keeps + // the catalog entry byte-identical for the overwhelming majority of + // manifests that declare no optional dependency. + if (optionalRequires.length > 0) { + result = { ...result, optional_requires: optionalRequires }; + } if (oauthProviders.length > 0) { result = { ...result, oauth_providers: oauthProviders }; } @@ -486,17 +499,19 @@ export function adaptManifestV1(doc: Record): Plugin | null { } /** - * Parses a `provides:` or `requires:` array. Each entry must be a non-empty - * string that {@link parseCapabilityRef} accepts. Malformed entries are - * dropped with a `console.warn` so that one bad manifest doesn't break - * catalog-load for the rest; the capability resolver additionally re-parses - * at activation time and surfaces a hard error if a `requires` has no - * provider — so dropping here is safe from a correctness standpoint. + * Parses a `provides:`, `requires:` or `optional_requires:` array. Each entry + * must be a non-empty string that {@link parseCapabilityRef} accepts — the + * three fields share one syntax so a capability can be moved between them + * without rewriting it. Malformed entries are dropped with a `console.warn` + * so that one bad manifest doesn't break catalog-load for the rest; the + * capability resolver additionally re-parses at activation time and surfaces + * a hard error if a `requires` has no provider — so dropping here is safe + * from a correctness standpoint. */ function extractCapabilityList( raw: unknown, pluginId: string, - field: 'provides' | 'requires', + field: 'provides' | 'requires' | 'optional_requires', ): string[] { const arr = asArray(raw); const out: string[] = []; diff --git a/middleware/src/routes/store.ts b/middleware/src/routes/store.ts index 3d5bfe17..7e348f1f 100644 --- a/middleware/src/routes/store.ts +++ b/middleware/src/routes/store.ts @@ -518,6 +518,13 @@ function registryEntryToPlugin(resolved: ResolvedRegistryPlugin): Plugin { jobs: [], provides: Array.isArray(summary.provides) ? summary.provides : [], requires: Array.isArray(summary.requires) ? summary.requires : [], + // #795 — optional prerequisites travel with the teaser so the consent UI + // can list them as "optional" rather than silently omitting them. Same + // defensive shape as the two above: an untrusted registry payload that + // sends a non-array is treated as "declared none". + ...(Array.isArray(summary.optional_requires) + ? { optional_requires: summary.optional_requires } + : {}), multi_instance: true, privacy_class: 'default', ...(setupGuide ? { setup_guide: setupGuide } : {}), diff --git a/middleware/test/coreMigrations.pg.test.ts b/middleware/test/coreMigrations.pg.test.ts new file mode 100644 index 00000000..f8714481 --- /dev/null +++ b/middleware/test/coreMigrations.pg.test.ts @@ -0,0 +1,216 @@ +/** + * #796 (epic #470 C9 / G3) — core's base schema must not depend on an LLM key. + * + * WHAT WAS WRONG + * -------------- + * `middleware/migrations/` is a core-owned directory whose only production + * caller lived inside the harness-orchestrator plugin's `activate()`, several + * hundred lines past an early return taken whenever no LLM provider resolves: + * + * const provider = await resolveLlmProvider(...); + * if (!provider) return { async close() {...} }; + * + * On a deployment with no provider key, core therefore had no schema at all. + * `_multi_orchestrator_migrations` did not exist, and neither did + * `plugin_public_path_grants` (C4's public-path consent table) nor + * `plugin_sql_grants` (C7's SQL consent table) — so recording either operator + * consent was structurally impossible. The failure was silent by + * construction: nothing logged a migration error, because no migration was + * ever attempted. The P5 acceptance run had to apply all 47 files by hand. + * + * WHAT THIS PINS + * -------------- + * `runCoreMigrations` is what core's boot now calls, before any tool plugin + * activates and with no provider configured. The suite deliberately runs with + * every provider key stripped from the environment, so a regression that + * reattached the ledger to a credential fails here rather than in staging. + * + * Isolation: each case builds its own schema and takes `public` off the + * search_path, so the 47 files apply against an empty namespace without + * touching the tables the other pg suites share. Schema names carry the + * `c9core_` prefix. + * + * Skips when no test Postgres is reachable, mirroring the other pg suites. + */ + +import { strict as assert } from 'node:assert'; +import { readdir } from 'node:fs/promises'; +import { dirname, resolve } from 'node:path'; +import { after, describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import { Pool } from 'pg'; + +import { runCoreMigrations } from '../src/platform/coreMigrations.js'; + +import { probePgTest } from './_helpers/pgTestDb.js'; + +const { url: PG_URL, reachable: pgAvailable } = await probePgTest({ + label: 'coreMigrations', + vars: ['GRAPH_PG_TEST_URL', 'MEMORY_PG_TEST_URL', 'WS5_PG_TEST_URL'], +}); + +const migrationsDir = resolve( + dirname(fileURLToPath(import.meta.url)), + '..', + 'migrations', +); + +/** Env vars any provider path could read. Cleared for the whole suite. */ +const PROVIDER_KEYS = [ + 'ANTHROPIC_API_KEY', + 'OPENAI_API_KEY', + 'AZURE_OPENAI_API_KEY', + 'GOOGLE_API_KEY', + 'LLM_PROVIDER_API_KEY', +] as const; + +const savedEnv = new Map(); +for (const key of PROVIDER_KEYS) { + savedEnv.set(key, process.env[key]); + delete process.env[key]; +} + +/** + * One capped pool for the suite's own assertions. ~16 other pg suites run + * concurrently, each holding a default-sized pool, so an uncapped extra pool + * here is enough to exhaust `max_connections` and cancel an unrelated suite. + */ +const probePool = pgAvailable + ? new Pool({ connectionString: PG_URL, max: 2, idleTimeoutMillis: 1_000 }) + : undefined; + +after(async () => { + await probePool?.end().catch(() => undefined); + for (const [key, value] of savedEnv) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } +}); + +/** + * Run `runCoreMigrations` against a scratch schema. + * + * The `createPool` seam exists for exactly this: the migrations are written + * unqualified, so pointing `search_path` at a private schema is what makes + * them land somewhere this suite may drop afterwards. + */ +async function migrateInto(schema: string): Promise { + await probePool?.query(`DROP SCHEMA IF EXISTS ${schema} CASCADE`); + await probePool?.query(`CREATE SCHEMA ${schema}`); + const outcome = await runCoreMigrations({ + databaseUrl: PG_URL ?? '', + createPool: (connectionString) => + new Pool({ + connectionString, + max: 2, + idleTimeoutMillis: 1_000, + options: `-c search_path=${schema}`, + }), + }); + return outcome; +} + +async function tableExists(schema: string, table: string): Promise { + const res = await probePool?.query<{ exists: boolean }>( + 'SELECT to_regclass($1) IS NOT NULL AS exists', + [`${schema}.${table}`], + ); + return res?.rows[0]?.exists === true; +} + +describe('#796 core migrations run at boot, without any LLM provider', () => { + it('is a no-op when there is no DATABASE_URL, not a crash', async () => { + // Tests and zero-config dev boot on the in-memory backend. Core must + // say so and continue, rather than throwing on a missing connection. + assert.equal(await runCoreMigrations({ databaseUrl: undefined }), 'no-database'); + assert.equal(await runCoreMigrations({ databaseUrl: ' ' }), 'no-database'); + assert.equal(await runCoreMigrations({}), 'no-database'); + }); + + it('applies the full core ledger with every provider key unset', { + skip: pgAvailable ? false : 'no test Postgres reachable', + }, async () => { + for (const key of PROVIDER_KEYS) { + assert.equal(process.env[key], undefined, `${key} must be unset here`); + } + + const schema = 'c9core_apply'; + try { + assert.equal(await migrateInto(schema), 'applied'); + + // The ledger itself exists and names every file on disk. Asserting the + // COUNT against the directory (rather than a hardcoded 47) keeps this + // honest as migrations are added. + const onDisk = (await readdir(migrationsDir)).filter((f) => + f.endsWith('.sql'), + ); + const ledger = await probePool?.query<{ n: string }>( + `SELECT count(*)::text AS n FROM ${schema}._multi_orchestrator_migrations`, + ); + assert.equal( + Number(ledger?.rows[0]?.n), + onDisk.length, + 'every migration on disk must be recorded in the ledger', + ); + + // The two tables the gap actually cost operators. Named individually + // rather than folded into the count above, because "the ledger ran" + // and "consent is recordable" are the two separate claims #796 makes. + assert.ok( + await tableExists(schema, 'plugin_public_path_grants'), + "C4's public-path consent table must exist", + ); + assert.ok( + await tableExists(schema, 'plugin_sql_grants'), + "C7's SQL-grant consent table must exist", + ); + } finally { + await probePool?.query(`DROP SCHEMA IF EXISTS ${schema} CASCADE`); + } + }); + + it('is idempotent — a second pass applies nothing and takes no lock', { + skip: pgAvailable ? false : 'no test Postgres reachable', + }, async () => { + const schema = 'c9core_second_pass'; + try { + await migrateInto(schema); + const first = await probePool?.query<{ n: string }>( + `SELECT count(*)::text AS n FROM ${schema}._multi_orchestrator_migrations`, + ); + + // This is the shape the harness-orchestrator plugin's retained call + // now has: core already ran the ledger at boot, so the plugin's own + // invocation must be a cheap no-op rather than a re-apply. + assert.equal(await migrateInto2(schema), 'applied'); + + const second = await probePool?.query<{ n: string }>( + `SELECT count(*)::text AS n FROM ${schema}._multi_orchestrator_migrations`, + ); + assert.equal(second?.rows[0]?.n, first?.rows[0]?.n); + + // Nothing was left holding the migration advisory lock. + const locks = await probePool?.query<{ n: string }>( + "SELECT count(*)::text AS n FROM pg_locks WHERE locktype = 'advisory' AND classid = 4410", + ); + assert.equal(Number(locks?.rows[0]?.n), 0, 'advisory lock must be released'); + } finally { + await probePool?.query(`DROP SCHEMA IF EXISTS ${schema} CASCADE`); + } + }); +}); + +/** Second run against an already-migrated schema (no DROP/CREATE first). */ +async function migrateInto2(schema: string): Promise { + return runCoreMigrations({ + databaseUrl: PG_URL ?? '', + createPool: (connectionString) => + new Pool({ + connectionString, + max: 2, + idleTimeoutMillis: 1_000, + options: `-c search_path=${schema}`, + }), + }); +} diff --git a/middleware/test/installServiceActivationTruthful.test.ts b/middleware/test/installServiceActivationTruthful.test.ts index 2bba5173..e2bc2e2d 100644 --- a/middleware/test/installServiceActivationTruthful.test.ts +++ b/middleware/test/installServiceActivationTruthful.test.ts @@ -9,7 +9,7 @@ * operator saw a green plugin serving nothing, and the only trace was one line * of stderr. * - * Found by installing the extracted dev-platform plugin: the registry answered + * Found by installing the first extracted plugin: the registry answered * `{"status":"active"}` while every one of its routes 404'd. * * The boot path has always done this correctly (`toolPluginRuntime.ts` calls @@ -75,7 +75,8 @@ async function install(service: InstallService, pluginId: string) { void describe('install reports activation truthfully (#470 P5)', () => { void it('marks the entry errored when the onInstalled hook throws', async () => { - const boom = "NativeToolRegistry: duplicate native-tool name 'dev_job_start'"; + const boom = + "NativeToolRegistry: duplicate native-tool name 'job_start'"; const { service, registry, pluginId } = makeService(async () => { throw new Error(boom); }); diff --git a/middleware/test/optionalRequires.test.ts b/middleware/test/optionalRequires.test.ts new file mode 100644 index 00000000..9fc29543 --- /dev/null +++ b/middleware/test/optionalRequires.test.ts @@ -0,0 +1,423 @@ +/** + * #795 (epic #470 C9) — `optional_requires:` — a dependency a plugin can + * survive the absence of. + * + * WHAT WAS UNREPRESENTABLE + * ----------------------- + * Two core rules met and left no legal spelling for a degradable dependency: + * + * - C2b makes `ctx.services.get(name)` throw `ServiceNotDeclaredError` for + * a name in neither `requires:` nor `provides:` — so a plugin MUST list + * anything it might resolve. + * - The installer (`InstallService.create`) and the boot loop + * (`resolveEligiblePlugins`) both treat every `requires:` entry as a hard + * prerequisite. + * + * Declaring a degradable dependency therefore blocked the install; omitting + * it made runtime resolution throw. `optional_requires:` is the third + * position: it satisfies the declaration gate and neither enforcement gate. + * + * These tests pin the properties that make it worth having, one per gate + * that had to agree: + * 1. the manifest loader parses it with the same capability-ref syntax; + * 2. it grants `services.get` / `services.getOptional` (no throw), and + * resolution answers `undefined` when nothing provides it; + * 3. a provider that IS registered resolves normally through it; + * 4. an UNdeclared name still throws — optionality is not a hole; + * 5. neither the installer nor the boot resolver treats it as a + * prerequisite, while `requires:` still does. The contrast is load + * bearing: a test that only checked the optional side would stay green + * against a build that had stopped enforcing `requires:` entirely. + * + * Mutation check, run while writing these: dropping + * `entry.plugin.optional_requires` from `declaredServiceNames` fails the + * three cases in block 2; making `walkCapabilityInstallChain` read optional + * entries fails the two install cases; restoring the optional edge in + * `resolveCapabilities` fails the last case. + */ + +import { strict as assert } from 'node:assert'; +import { describe, it } from 'node:test'; + +import { ServiceNotDeclaredError } from '@omadia/plugin-api'; + +import type { Plugin } from '../src/api/admin-v1.js'; +import { createPluginContext } from '../src/platform/pluginContext.js'; +import type { CreatePluginContextOptions } from '../src/platform/pluginContext.js'; +import { + classifyServiceGrant, + declaredServiceNames, +} from '../src/platform/pluginServiceGrants.js'; +import { ServiceRegistry } from '../src/platform/serviceRegistry.js'; +import { + resolveEligiblePlugins, + walkCapabilityInstallChain, +} from '../src/plugins/capabilityResolver.js'; +import type { + InstalledAgent, + InstalledRegistry, +} from '../src/plugins/installedRegistry.js'; +import { InstallError, InstallService } from '../src/plugins/installService.js'; +import { adaptManifestV1 } from '../src/plugins/manifestLoader.js'; +import type { PluginCatalog } from '../src/plugins/manifestLoader.js'; +import type { SecretVault } from '../src/secrets/vault.js'; + +// --- fixtures -------------------------------------------------------------- + +interface PluginSpec { + readonly id: string; + readonly requires?: string[]; + readonly optional_requires?: string[]; + readonly provides?: string[]; +} + +/** + * Built through the real `adaptManifestV1`, never as an object literal. + * The feature under test is "does the loader carry this field through to the + * gates", so a fixture that bypassed the loader could stay green while the + * loader silently dropped `optional_requires:`. + */ +function pluginOf(spec: PluginSpec): Plugin { + const plugin = adaptManifestV1({ + schema_version: '1', + identity: { + id: spec.id, + name: spec.id, + version: '1.0.0', + kind: 'extension', + domain: 'test.optional', + }, + ...(spec.requires ? { requires: spec.requires } : {}), + ...(spec.optional_requires + ? { optional_requires: spec.optional_requires } + : {}), + ...(spec.provides ? { provides: spec.provides } : {}), + }); + assert.ok(plugin, `fixture manifest for ${spec.id} must adapt`); + return plugin; +} + +function catalogOf(...plugins: Plugin[]): PluginCatalog { + const entries = new Map( + plugins.map((plugin) => [ + plugin.id, + { + plugin, + manifest: {}, + source_path: 'test', + source_kind: 'manifest-v1', + }, + ]), + ); + return { + get: (id: string) => entries.get(id), + list: () => [...entries.values()], + } as unknown as PluginCatalog; +} + +function makeCtx( + agentId: string, + catalog: PluginCatalog, + registry = new ServiceRegistry(), +): { ctx: ReturnType; registry: ServiceRegistry } { + const stub = (): (() => void) => (): void => {}; + const ctx = createPluginContext({ + agentId, + vault: { + get: async (): Promise => undefined, + listKeys: async (): Promise => [], + }, + registry: { has: () => true, list: () => [], get: () => undefined }, + catalog, + serviceRegistry: registry, + nativeToolRegistry: { register: stub, registerHandler: stub }, + routeRegistry: { register: stub, disposeBySource: () => 0 }, + jobScheduler: { register: stub, stopForPlugin: (): void => {} }, + notificationRouter: { dispatch: (): void => {}, registerChannel: stub }, + uiRouteCatalog: { register: stub, registerNav: stub }, + logger: (): void => {}, + } as unknown as CreatePluginContextOptions); + return { ctx, registry }; +} + +function installedRegistry(active: readonly string[] = []): InstalledRegistry { + const map = new Map(); + for (const id of active) { + map.set(id, { + id, + installed_version: '1.0.0', + installed_at: '2026-08-20T00:00:00Z', + status: 'active', + config: {}, + }); + } + return { + list: () => [...map.values()], + get: (id) => map.get(id), + has: (id) => map.has(id), + register: async () => { + /* no-op */ + }, + remove: async () => { + /* no-op */ + }, + markActivationFailed: async () => { + /* no-op */ + }, + markActivationSucceeded: async () => { + /* no-op */ + }, + clearActivationError: async () => { + /* no-op */ + }, + updateConfig: async () => { + /* no-op */ + }, + updateVersion: async () => { + /* no-op */ + }, + }; +} + +const noopVault = { + setMany: async () => { + /* no-op */ + }, + getMany: async () => ({}), + purge: async () => { + /* no-op */ + }, + list: async () => [], +} as unknown as SecretVault; + +// --- 1. the manifest loader ------------------------------------------------ + +describe('#795 manifest — optional_requires parses like requires', () => { + it('carries valid capability-refs onto the Plugin, separate from requires', () => { + const plugin = pluginOf({ + id: '@test/consumer', + requires: ['knowledgeGraph@^1'], + optional_requires: ['turnContext@1', 'usageTelemetry@^1'], + }); + assert.deepEqual(plugin.requires, ['knowledgeGraph@^1']); + assert.deepEqual(plugin.optional_requires, [ + 'turnContext@1', + 'usageTelemetry@^1', + ]); + }); + + it('omits the field entirely when the manifest declares none', () => { + const plugin = pluginOf({ id: '@test/plain', requires: [] }); + assert.equal(plugin.optional_requires, undefined); + }); + + it('drops a malformed entry rather than failing the whole manifest', () => { + const plugin = pluginOf({ + id: '@test/messy', + optional_requires: ['turnContext@1', 'no-version-here'], + }); + assert.deepEqual(plugin.optional_requires, ['turnContext@1']); + }); +}); + +// --- 2 + 3 + 4. the C2b declaration gate ---------------------------------- + +describe('#795 services gate — optional_requires is a declaration', () => { + it('counts toward declaredServiceNames alongside requires and provides', () => { + const catalog = catalogOf( + pluginOf({ + id: '@test/consumer', + requires: ['knowledgeGraph@^1'], + optional_requires: ['turnContext@1'], + provides: ['reportStore@1'], + }), + ); + const declared = declaredServiceNames('@test/consumer', catalog); + assert.ok(declared.has('knowledgeGraph')); + assert.ok(declared.has('turnContext'), 'optional_requires must grant'); + assert.ok(declared.has('reportStore')); + }); + + it('classifies an optional-only capability as declared, not undeclared', () => { + const catalog = catalogOf( + pluginOf({ id: '@test/consumer', optional_requires: ['turnContext@1'] }), + ); + const declared = declaredServiceNames('@test/consumer', catalog); + assert.equal( + classifyServiceGrant('@test/consumer', 'turnContext', declared, catalog), + 'declared', + ); + }); + + it('declared-optional + provider ABSENT: getOptional answers undefined, get does not throw', () => { + const catalog = catalogOf( + pluginOf({ id: '@test/consumer', optional_requires: ['turnContext@1'] }), + ); + const { ctx } = makeCtx('@test/consumer', catalog); + + // The whole point: absence is an answer, not an exception. + assert.equal(ctx.services.getOptional('turnContext'), undefined); + assert.doesNotThrow(() => ctx.services.get('turnContext')); + assert.equal(ctx.services.get('turnContext'), undefined); + assert.equal(ctx.services.has('turnContext'), false); + }); + + it('declared-optional + provider PRESENT: resolves through both accessors', () => { + const catalog = catalogOf( + pluginOf({ id: '@test/consumer', optional_requires: ['turnContext@1'] }), + ); + const { ctx, registry } = makeCtx('@test/consumer', catalog); + const impl = { currentTurnId: (): string => 'turn-1' }; + registry.provide('turnContext', impl); + + assert.equal(ctx.services.getOptional('turnContext'), impl); + assert.equal(ctx.services.get('turnContext'), impl); + assert.equal(ctx.services.has('turnContext'), true); + }); + + it('UNDECLARED still throws through getOptional — optionality is not a hole', () => { + const catalog = catalogOf( + pluginOf({ id: '@test/consumer', optional_requires: ['turnContext@1'] }), + ); + const { ctx, registry } = makeCtx('@test/consumer', catalog); + registry.provide('graphPool', { pool: 'the real one' }); + + // A typo must not become `undefined`: that is exactly the failure the + // declaration gate exists to prevent, and `getOptional` inherits it. + assert.throws( + () => ctx.services.getOptional('graphPool'), + (err: unknown) => { + assert.ok(err instanceof ServiceNotDeclaredError); + assert.equal(err.capability, 'graphPool'); + return true; + }, + ); + assert.throws( + () => ctx.services.getOptional('turnContxet'), + ServiceNotDeclaredError, + ); + }); + + it('names optional_requires in the error a plugin author has to act on', () => { + const catalog = catalogOf(pluginOf({ id: '@test/consumer' })); + const { ctx } = makeCtx('@test/consumer', catalog); + assert.throws( + () => ctx.services.get('turnContext'), + (err: unknown) => { + assert.ok(err instanceof ServiceNotDeclaredError); + assert.match(err.message, /optional_requires:/); + return true; + }, + ); + }); +}); + +// --- 5. the two enforcement gates ----------------------------------------- + +describe('#795 install gate — optional_requires never yields a 409', () => { + const service = ( + catalog: PluginCatalog, + active: readonly string[] = [], + ): InstallService => + new InstallService({ + catalog, + registry: installedRegistry(active), + vault: noopVault, + }); + + it('installs a plugin whose optional capability nothing provides', () => { + const catalog = catalogOf( + pluginOf({ + id: '@test/consumer', + optional_requires: ['turnContext@1', 'usageTelemetry@^1'], + }), + ); + const job = service(catalog).create('@test/consumer'); + assert.equal(job.plugin_id, '@test/consumer'); + assert.equal(job.state, 'awaiting_config'); + }); + + it('still 409s for a hard requires — the contrast is the assertion', () => { + const catalog = catalogOf( + pluginOf({ + id: '@test/consumer', + requires: ['knowledgeGraph@^1'], + optional_requires: ['turnContext@1'], + }), + pluginOf({ id: '@test/kg', provides: ['knowledgeGraph@1'] }), + ); + assert.throws( + () => service(catalog).create('@test/consumer'), + (err: unknown) => { + assert.ok(err instanceof InstallError); + assert.equal(err.code, 'install.missing_capability'); + assert.equal(err.status, 409); + return true; + }, + ); + }); + + it('keeps the optional capability out of the unresolved chain entirely', () => { + const catalog = catalogOf( + pluginOf({ + id: '@test/consumer', + requires: ['knowledgeGraph@^1'], + optional_requires: ['turnContext@1'], + }), + pluginOf({ id: '@test/kg', provides: ['knowledgeGraph@1'] }), + ); + const chain = walkCapabilityInstallChain( + '@test/consumer', + catalog, + installedRegistry(), + ); + assert.deepEqual(chain.unresolved_requires, ['knowledgeGraph@^1']); + assert.ok( + !chain.available_providers.some((p) => p.capability === 'turnContext@1'), + 'an optional capability must not appear in available_providers — the ' + + 'operator would be told to install a provider for something the ' + + 'plugin already said it can live without', + ); + }); +}); + +describe('#795 boot resolver — optional_requires is not an activation dep', () => { + it('keeps a consumer eligible when only its optional capability is missing', () => { + const catalog = catalogOf( + pluginOf({ id: '@test/consumer', optional_requires: ['turnContext@1'] }), + ); + const resolution = resolveEligiblePlugins(['@test/consumer'], catalog); + assert.deepEqual(resolution.resolved, ['@test/consumer']); + assert.deepEqual(resolution.unresolved, []); + }); + + it('drops the same consumer when the capability is a hard require', () => { + const catalog = catalogOf( + pluginOf({ id: '@test/consumer', requires: ['turnContext@1'] }), + ); + const resolution = resolveEligiblePlugins(['@test/consumer'], catalog); + assert.deepEqual(resolution.resolved, []); + assert.equal(resolution.unresolved.length, 1); + assert.deepEqual(resolution.unresolved[0]?.requires, ['turnContext@1']); + }); + + it('contributes no ordering edge even when the provider IS eligible', () => { + const catalog = catalogOf( + pluginOf({ id: '@test/consumer', optional_requires: ['turnContext@1'] }), + pluginOf({ id: '@test/provider', provides: ['turnContext@1'] }), + ); + const resolution = resolveEligiblePlugins( + ['@test/consumer', '@test/provider'], + catalog, + ); + assert.deepEqual( + [...resolution.resolved].sort(), + ['@test/consumer', '@test/provider'], + ); + // Deliberate, and documented in capabilityResolver.ts: an edge from a + // link the kernel may not enforce would turn a mutual optional + // reference into a topo-sort cycle — a boot failure caused by a + // dependency the manifest declared as skippable. + assert.deepEqual(resolution.edges, []); + }); +}); diff --git a/middleware/test/uiRouteCatalogPluginUiNav.test.ts b/middleware/test/uiRouteCatalogPluginUiNav.test.ts new file mode 100644 index 00000000..b8970b4f --- /dev/null +++ b/middleware/test/uiRouteCatalogPluginUiNav.test.ts @@ -0,0 +1,222 @@ +/** + * #798 (epic #470 C9) — a scoped plugin id could not express a nav href. + * + * THE CONTRADICTION + * ----------------- + * Core serves a plugin's bundled UI at `/p/:pluginId/ui/`, and Express + * splits on a raw `/` — so `@acme/widget` only resolves percent-encoded + * (`%40acme%2Fwidget`). Measured on a live core during the P5 acceptance + * run: encoded 200, raw 404. + * + * `HREF_SEGMENT` rejects `%`, and rightly: the shell decides "core + * destinations win" by comparing hrefs for string equality, which + * percent-encoding defeats. So the only URL that worked was the only one + * the validator refused, and the one it accepted 404'd in the browser. + * Every `@scope/name` plugin hit it. + * + * THE FIX UNDER TEST + * ------------------ + * The plugin states intent (`pluginUi: true`) and the kernel renders the + * path from the id it already holds. The literal-href validator is + * untouched — asserted explicitly below, because the tempting fix + * (widening `HREF_SEGMENT` to admit `%xx`, which is what the acceptance run + * patched locally to get unblocked) would weaken every literal href in + * order to fix one path core can spell for itself. + * + * Mutation check, run while writing these: routing a `pluginUi` entry + * through `assertInAppHref` fails case 1; dropping the flag from `listNav` + * fails case 3; accepting `href` and `pluginUi` together fails case 5; + * skipping the id charset check fails case 8. + */ + +import { strict as assert } from 'node:assert'; +import { readFile } from 'node:fs/promises'; +import { dirname, resolve } from 'node:path'; +import { describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import { + UiRouteCatalog, + pluginUiHref, +} from '../src/platform/uiRouteCatalog.js'; + +const label = { en: 'Reports' } as const; + +const srcDir = resolve(dirname(fileURLToPath(import.meta.url)), '..', 'src'); + +/** + * `uiRouteCatalog.ts` restates the manifest's plugin-id gate rather than + * importing it, for the same reason `web-ui/app/_lib/pluginId.ts` does + * (C8b): `manifestLoader.ts` keeps those declarations in the exact source + * form a parity test anchors on, and exporting them would reformat the very + * lines being matched. + * + * So the restatement is pinned the same way. A comment claiming two + * definitions agree is worth nothing; this makes drift fail a test rather + * than a nav entry. + */ +describe('#798 plugin-id gate parity with manifestLoader', () => { + it('restates PLUGIN_ID_PATTERN character-identically', async () => { + const authority = await readFile( + resolve(srcDir, 'plugins', 'manifestLoader.ts'), + 'utf8', + ); + const copy = await readFile( + resolve(srcDir, 'platform', 'uiRouteCatalog.ts'), + 'utf8', + ); + const authoritative = /^const PLUGIN_ID_PATTERN = (\/.*\/);$/m.exec( + authority, + ); + assert.ok( + authoritative, + 'could not find `const PLUGIN_ID_PATTERN = /…/;` in manifestLoader.ts — ' + + 'if it was renamed or reformatted, update this test and the copy in ' + + 'uiRouteCatalog.ts together', + ); + const restated = /^const ENCODABLE_PLUGIN_ID =\n {2}(\/.*\/);$/m.exec(copy); + assert.ok(restated, 'could not find ENCODABLE_PLUGIN_ID in uiRouteCatalog.ts'); + assert.equal(restated[1], authoritative[1]); + }); + + it('restates PLUGIN_ID_MAX_LENGTH character-identically', async () => { + const authority = await readFile( + resolve(srcDir, 'plugins', 'manifestLoader.ts'), + 'utf8', + ); + const copy = await readFile( + resolve(srcDir, 'platform', 'uiRouteCatalog.ts'), + 'utf8', + ); + const a = /^const PLUGIN_ID_MAX_LENGTH = (\d+);$/m.exec(authority); + const b = /^const PLUGIN_ID_MAX_LENGTH = (\d+);$/m.exec(copy); + assert.ok(a && b); + assert.equal(b[1], a[1]); + }); +}); + +describe('UiRouteCatalog — pluginUi nav entries (#798)', () => { + it('renders the canonical encoded host-page path for a scoped plugin id', () => { + const cat = new UiRouteCatalog(); + cat.registerNav('@acme/widget', { + navId: 'main', + pluginUi: true, + cluster: 'adminCluster', + label, + }); + + const entry = cat.listNav('en')[0]; + assert.equal(entry?.href, '/plugin-ui/%40acme%2Fwidget'); + assert.equal(entry?.pluginId, '@acme/widget'); + assert.equal( + entry?.href, + pluginUiHref('@acme/widget'), + 'the catalogue and the exported helper must agree on one spelling — ' + + 'the web UI mirrors this derivation and a second literal would drift', + ); + }); + + it('leaves an unscoped id unencoded — encoding is not blanket-applied', () => { + const cat = new UiRouteCatalog(); + cat.registerNav('reporter', { navId: 'main', pluginUi: true, label }); + assert.equal(cat.listNav('en')[0]?.href, '/plugin-ui/reporter'); + }); + + it('flags the entry so the shell can re-derive the href itself', () => { + const cat = new UiRouteCatalog(); + cat.registerNav('@acme/widget', { navId: 'main', pluginUi: true, label }); + assert.equal(cat.listNav('en')[0]?.pluginUi, true); + }); + + it('does NOT flag a literal-href entry', () => { + const cat = new UiRouteCatalog(); + cat.registerNav('@acme/widget', { + navId: 'main', + href: '/admin/reports', + label, + }); + const entry = cat.listNav('en')[0]; + assert.equal(entry?.href, '/admin/reports'); + assert.equal(entry?.pluginUi, undefined); + }); + + it('refuses href and pluginUi together — the destination must be unambiguous', () => { + const cat = new UiRouteCatalog(); + assert.throws( + () => + cat.registerNav('@acme/widget', { + navId: 'main', + href: '/admin/reports', + pluginUi: true, + label, + }), + /either 'href' or 'pluginUi: true', not both/, + ); + }); + + it('refuses an entry with neither', () => { + const cat = new UiRouteCatalog(); + assert.throws( + () => cat.registerNav('@acme/widget', { navId: 'main', label }), + /needs a destination/, + ); + }); + + it('refuses a non-literal-true pluginUi rather than coercing it', () => { + const cat = new UiRouteCatalog(); + assert.throws( + () => + cat.registerNav('@acme/widget', { + navId: 'main', + pluginUi: 'yes' as unknown as true, + label, + }), + /pluginUi must be literal/, + ); + }); + + it('refuses to encode a plugin id that never passed the manifest charset gate', () => { + const cat = new UiRouteCatalog(); + for (const badId of ['../etc', 'Has Spaces', '@acme/UPPER', 'a/b/c']) { + assert.throws( + () => cat.registerNav(badId, { navId: 'main', pluginUi: true, label }), + /npm-style plugin id/, + `id '${badId}' must not reach the shell's chrome percent-encoded`, + ); + } + }); + + it('KEEPS the literal-href validator strict — percent-encoding still refused', () => { + const cat = new UiRouteCatalog(); + assert.throws( + () => + cat.registerNav('@acme/widget', { + navId: 'main', + href: '/plugin-ui/%40acme%2Fwidget', + label, + }), + /percent-encoding/, + ); + assert.throws( + () => + cat.registerNav('@acme/widget', { + navId: 'other', + href: '/x/%2e%2e/admin', + label, + }), + /percent-encoding/, + ); + }); + + it('disposes a pluginUi entry like any other', () => { + const cat = new UiRouteCatalog(); + const dispose = cat.registerNav('@acme/widget', { + navId: 'main', + pluginUi: true, + label, + }); + assert.equal(cat.navSize(), 1); + dispose(); + assert.equal(cat.navSize(), 0); + }); +}); diff --git a/specs/470-dev-platform-plugin/decoupling-baseline.json b/specs/470-dev-platform-plugin/decoupling-baseline.json index ff3bec6e..1eefcc36 100644 --- a/specs/470-dev-platform-plugin/decoupling-baseline.json +++ b/specs/470-dev-platform-plugin/decoupling-baseline.json @@ -1,9 +1,9 @@ { - "total": 3300, + "total": 3299, "zones": { "middleware/src": 1576, - "middleware/test": 1030, - "middleware/packages": 89, + "middleware/test": 1029, + "middleware/packages": 88, "middleware/scripts": 8, "middleware/sidecars": 195, "middleware/migrations": 69, diff --git a/web-ui/app/_lib/__tests__/navParse.test.ts b/web-ui/app/_lib/__tests__/navParse.test.ts index dbe9a51b..bf79e2c8 100644 --- a/web-ui/app/_lib/__tests__/navParse.test.ts +++ b/web-ui/app/_lib/__tests__/navParse.test.ts @@ -111,3 +111,82 @@ describe('parseEntries', () => { expect(parsed[0]?.href).toBe('/admin/dev-platform'); }); }); + +/** + * #798 — plugin-UI nav entries. + * + * A scoped plugin id only resolves percent-encoded, and percent-encoding is + * exactly what the canonical-href rule above refuses. So the middleware flags + * such an entry with `pluginUi: true` and this file DERIVES the href from + * `pluginId` instead of validating the transmitted string. + * + * That is deliberately stronger than validating would have been: the only + * encoded path this module can emit is one it computed itself from a + * charset-checked id, so a version skew or a compromised control plane still + * cannot inject an arbitrary encoded path into the trusted header. + * + * Mutation check: making the parser trust the incoming `href` for a + * `pluginUi` entry fails "ignores the transmitted href". + */ +describe('parseEntries — pluginUi entries (#798)', () => { + const pluginUiEntry = { + pluginId: '@acme/widget', + navId: 'main', + href: '/plugin-ui/%40acme%2Fwidget', + label: 'Reports', + order: 50, + cluster: 'adminCluster', + pluginUi: true, + }; + + it('accepts a scoped-id entry the canonical-href rule would reject', () => { + const parsed = parseEntries(wrap(pluginUiEntry)); + expect(parsed).toHaveLength(1); + expect(parsed[0]?.href).toBe('/plugin-ui/%40acme%2Fwidget'); + expect(parsed[0]?.pluginId).toBe('@acme/widget'); + expect(parsed[0]?.cluster).toBe('adminCluster'); + }); + + it('rejects that same href when the entry is NOT flagged pluginUi', () => { + // The strict rule is untouched for literal hrefs — the flag is the only + // thing that admits percent-encoding, and only for a path core computes. + expect( + parseEntries(wrap({ ...pluginUiEntry, pluginUi: undefined })), + ).toEqual([]); + }); + + it('ignores the transmitted href and derives it from pluginId', () => { + const parsed = parseEntries( + wrap({ ...pluginUiEntry, href: '/admin/somewhere-else' }), + ); + expect(parsed[0]?.href).toBe('/plugin-ui/%40acme%2Fwidget'); + }); + + it('leaves an unscoped id unencoded', () => { + const parsed = parseEntries( + wrap({ ...pluginUiEntry, pluginId: 'reporter' }), + ); + expect(parsed[0]?.href).toBe('/plugin-ui/reporter'); + }); + + it('drops an entry whose pluginId is not an npm-style id', () => { + for (const pluginId of ['../etc', 'Has Spaces', '@acme/UPPER', 'a/b/c']) { + expect(parseEntries(wrap({ ...pluginUiEntry, pluginId }))).toEqual([]); + } + }); + + it('drops a pluginUi value that is not literal true', () => { + for (const pluginUi of ['yes', 1, {}, false]) { + expect(parseEntries(wrap({ ...pluginUiEntry, pluginUi }))).toEqual([]); + } + }); + + it('still enforces the label rules on a pluginUi entry', () => { + expect( + parseEntries(wrap({ ...pluginUiEntry, label: 'x'.repeat(41) })), + ).toEqual([]); + expect( + parseEntries(wrap({ ...pluginUiEntry, label: 'Rep\u202eorts' })), + ).toEqual([]); + }); +}); diff --git a/web-ui/app/_lib/navigation.ts b/web-ui/app/_lib/navigation.ts index 0d980899..da107c25 100644 --- a/web-ui/app/_lib/navigation.ts +++ b/web-ui/app/_lib/navigation.ts @@ -18,6 +18,8 @@ * conventions ever change. */ +import { isValidPluginId } from './pluginId'; + function botApi(path: string): string { if (typeof window !== 'undefined') { return `/bot-api${path}`; @@ -60,6 +62,25 @@ const MAX_HREF_LENGTH = 256; const MAX_ENTRIES = 100; const HREF_SEGMENT = /^[A-Za-z0-9\-._~]+$/; +/** + * The canonical host-page path for a plugin's own bundled UI (#798). + * + * A scoped plugin id only resolves percent-encoded — and percent-encoding is + * exactly what {@link isCanonicalInAppHref} refuses, on purpose, because the + * shell compares hrefs to core paths by string equality. Rather than relax + * that rule for one case, an entry flagged `pluginUi` has its href DERIVED + * here from `pluginId` and the transmitted href is discarded. + * + * That is a stronger check than validating the string would have been. The + * middleware is a separate deployable; this way a version skew or a + * compromised control plane cannot put an arbitrary encoded path into the + * chrome, because the only encoded path this file can produce is the one it + * computes itself from a charset-checked id. + */ +function pluginUiHref(pluginId: string): string { + return `/plugin-ui/${encodeURIComponent(pluginId)}`; +} + /** Control, bidi-formatting and zero-width codepoints. */ function hasUnsafeChars(value: string): boolean { for (let i = 0; i < value.length; i += 1) { @@ -109,7 +130,7 @@ export function parseEntries(payload: unknown): readonly NavEntryDto[] { for (const item of raw.slice(0, MAX_ENTRIES)) { if (typeof item !== 'object' || item === null) continue; const e = item as Record; - const { pluginId, navId, href, label, order, cluster } = e; + const { pluginId, navId, href, label, order, cluster, pluginUi } = e; if ( typeof pluginId !== 'string' || typeof navId !== 'string' || @@ -119,9 +140,31 @@ export function parseEntries(payload: unknown): readonly NavEntryDto[] { continue; } if (label.trim().length === 0 || label.length > MAX_LABEL_LENGTH) continue; - if (hasUnsafeChars(label) || hasUnsafeChars(href)) continue; - if (!isCanonicalInAppHref(href)) continue; + if (hasUnsafeChars(label)) continue; if (cluster !== undefined && typeof cluster !== 'string') continue; + + // #798 — a plugin-UI entry names its destination by plugin id, so the + // href is recomputed rather than validated. Anything else the payload + // claimed for `href` is ignored. + let resolvedHref: string; + if (pluginUi === true) { + // `isValidPluginId` rather than a fourth copy of the pattern: it is + // already pinned character-identical to `manifestLoader.ts` by + // `pluginId.test.ts` (C8b), and it is the same gate the host page at + // `/plugin-ui/[pluginId]` applies to whatever this href points at. + // A nav entry that passed here but 404'd there would be the exact + // class of split-brain bug #798 is about. + if (!isValidPluginId(pluginId)) continue; + resolvedHref = pluginUiHref(pluginId); + } else if (pluginUi !== undefined) { + // Present but not literal `true` — a shape this shell does not + // understand. Dropping beats guessing. + continue; + } else { + if (hasUnsafeChars(href)) continue; + if (!isCanonicalInAppHref(href)) continue; + resolvedHref = href; + } // `JSON.parse('{"order":1e400}')` yields Infinity, which is a number — // it would poison every comparison in the merge sort. const resolvedOrder = @@ -129,7 +172,7 @@ export function parseEntries(payload: unknown): readonly NavEntryDto[] { out.push({ pluginId, navId, - href, + href: resolvedHref, label, order: resolvedOrder, ...(typeof cluster === 'string' ? { cluster } : {}), From e4b16055ae2be8602d5147fba0ec531a759247e0 Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Thu, 20 Aug 2026 20:54:55 +0200 Subject: [PATCH 2/2] fix(#796): retry a contended migration lock at boot instead of exiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving `middleware/migrations/` out of the orchestrator plugin's `activate()` also moved it out from behind a catch, and the migrator's lock budget did not move with it. `runMultiOrchestratorMigrations` waits 2s for the `_multi_orchestrator_migrations` advisory lock, re-reads the ledger, and throws if work is still owed. That budget was sized for its old call site: inside `activate()`, capped at 10s by ToolPluginRuntime, where `activateAllInstalled` caught the throw per plugin, marked that one plugin errored and let boot continue — and where the "timed out" wording was chosen so `bootstrap.retryErroredPlugins` would classify it as transient and re-attempt on the next boot. At boot there is no such catch. The throw reaches `main().catch` and becomes `process.exit(1)`, so a cold multi-replica boot with 47 files to apply turned a survivable, self-healing race into a crash loop. Lock contention specifically is now retried, bounded at 60s. Each attempt re-enters the migrator, which re-reads the ledger, so once the winner commits the next attempt takes the migrator's own "applied by another replica while waiting" path and returns clean. Every other error still propagates on its first occurrence — a broken SQL file must fail loudly, which is the point of #796. Past the budget the migrator's error is rethrown unchanged. The predicate matches the migrator's message because it exports no error type; that coupling is pinned by a test that reads migrator.ts rather than trusted. Also adds the boot-wiring test the pg suite structurally cannot provide. `coreMigrations.pg.test.ts` calls `runCoreMigrations` directly, so reverting the `index.ts` wiring — deleting the call or putting it back behind a provider key — left it fully green. The regression #796 is about lives in the wiring. The new test pins that `runCoreMigrations` is awaited before `activateAllInstalled()` and is not gated on a credential. Mutation check: removing the retry reddens 4, moving the call after `activateAllInstalled` reddens 4, breaking the pinned phrase reddens 7. --- middleware/src/platform/coreMigrations.ts | 83 ++++++++- .../test/coreMigrationsBootWiring.test.ts | 163 ++++++++++++++++++ 2 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 middleware/test/coreMigrationsBootWiring.test.ts diff --git a/middleware/src/platform/coreMigrations.ts b/middleware/src/platform/coreMigrations.ts index cfedf6e5..07ac4389 100644 --- a/middleware/src/platform/coreMigrations.ts +++ b/middleware/src/platform/coreMigrations.ts @@ -48,6 +48,30 @@ import { runMultiOrchestratorMigrations } from '@omadia/orchestrator'; * so the steady-state boot is one SELECT and the orchestrator's own later call * is a no-op second pass. Multiple replicas booting together serialise on the * same lock they always did. + * + * LOCK CONTENTION + * --------------- + * The migrator gives up on the advisory lock after + * `MULTI_ORCH_MIGRATION_LOCK_WAIT_MS` (2s) and throws. That budget was sized + * for its ORIGINAL call site — inside the orchestrator plugin's `activate()`, + * which `ToolPluginRuntime` hard-caps at 10s, and where the throw was caught + * per-plugin: `activateAllInstalled` logged it, marked that one plugin + * errored, and boot continued. The wording ("timed out") was even chosen so + * `bootstrap.retryErroredPlugins` would classify it as transient and + * re-attempt on the next boot. + * + * Here there is no such catch: this runs at top level in `main()`, so an + * escaping throw becomes `process.exit(1)`. Moving the call without moving + * that assumption would convert a survivable, self-healing lock race into a + * boot crash — a cold multi-replica boot has 47 files to apply, and "the + * winner finishes inside 2s" is not a contract anyone can offer. + * + * So contention specifically is retried here, up to + * {@link LOCK_CONTENTION_TOTAL_WAIT_MS}. Each attempt re-enters the migrator, + * which re-reads the ledger — so once the winner commits, the next attempt + * takes the migrator's own "applied by another replica while waiting" path and + * returns clean. Every other error still propagates on its first occurrence: + * core without its schema must fail loudly, which is the entire point of #796. */ /** Outcome of a boot-time core-migration run. Returned rather than logged-only @@ -70,6 +94,46 @@ export interface CoreMigrationsOptions { * the environment. */ readonly createPool?: ((connectionString: string) => Pool) | undefined; + /** + * Migration-runner seam. Defaults to the real + * `runMultiOrchestratorMigrations`; overridden in tests that need to drive + * the lock-contention retry without racing two real boots against one + * database. + */ + readonly runMigrations?: + | ((pool: Pool, log: (msg: string) => void) => Promise) + | undefined; +} + +/** + * How long boot keeps re-attempting while another replica holds the migration + * lock. Generous on purpose: the cost of waiting is a slower boot, the cost of + * giving up early is a crash loop that competes with the replica actually + * making progress. Past this, the migrator's own error propagates unchanged. + */ +const LOCK_CONTENTION_TOTAL_WAIT_MS = 60_000; +/** Pause between attempts. The migrator already spends its own 2s inside each + * attempt waiting on the lock, so this only spaces the retries out. */ +const LOCK_CONTENTION_RETRY_DELAY_MS = 500; + +/** + * Does this error mean "another replica is mid-migration" rather than "the + * migration is broken"? Matched on the migrator's message because it exports + * no error type — and pinned by `coreMigrationsBootWiring.test.ts`, which + * reads `migrator.ts` and fails if that phrase stops being produced. Only + * contention is retryable; a failed SQL file must surface on attempt one. + */ +function isLockContentionError(err: unknown): boolean { + return ( + err instanceof Error && + err.message.includes( + 'waiting for the _multi_orchestrator_migrations advisory lock', + ) + ); +} + +function sleep(ms: number): Promise { + return new Promise((done) => setTimeout(done, ms)); } export async function runCoreMigrations( @@ -88,10 +152,25 @@ export async function runCoreMigrations( // long-lived pools plugins open later. new Pool({ connectionString, max: 2, idleTimeoutMillis: 1_000 })); + const runMigrations = opts.runMigrations ?? runMultiOrchestratorMigrations; + const pool = createPool(databaseUrl); + const deadline = Date.now() + LOCK_CONTENTION_TOTAL_WAIT_MS; try { - await runMultiOrchestratorMigrations(pool, log); - return 'applied'; + for (;;) { + try { + await runMigrations(pool, log); + return 'applied'; + } catch (err) { + const remaining = deadline - Date.now(); + if (!isLockContentionError(err) || remaining <= 0) throw err; + log( + '[middleware] core migrations: another replica holds the migration lock — ' + + `retrying for up to ${String(Math.ceil(remaining / 1_000))}s`, + ); + await sleep(Math.min(LOCK_CONTENTION_RETRY_DELAY_MS, remaining)); + } + } } finally { // `end()` must not mask a migration failure, and must not itself fail the // boot: the migrations are already committed by the time we get here. diff --git a/middleware/test/coreMigrationsBootWiring.test.ts b/middleware/test/coreMigrationsBootWiring.test.ts new file mode 100644 index 00000000..ce8d3b79 --- /dev/null +++ b/middleware/test/coreMigrationsBootWiring.test.ts @@ -0,0 +1,163 @@ +/** + * #796 (epic #470 C9 / G3) — the two claims `coreMigrations.pg.test.ts` cannot make. + * + * That suite proves `runCoreMigrations` applies the ledger with every provider + * key unset. It calls the function directly, so it stays green no matter what + * `index.ts` does with it — including reverting the boot call entirely, or + * putting it back behind an LLM credential. The regression #796 is actually + * about lives in the WIRING, and nothing was asserting the wiring. + * + * So this file pins the two properties the fix depends on and the pg suite + * structurally cannot see: + * + * 1. core's migrations run BEFORE `activateAllInstalled()`, and are not + * conditional on a provider key — read out of `index.ts` source, because + * importing `index.ts` boots the whole middleware. + * 2. lock contention at boot is retried rather than allowed to reach + * `main().catch` → `process.exit(1)`. + * + * The source-reading style is the same one `uiRouteCatalogPluginUiNav.test.ts` + * uses for its plugin-id parity pin: a claim about another file is worth + * nothing as a comment and something as a test. + */ + +import { strict as assert } from 'node:assert'; +import { readFile } from 'node:fs/promises'; +import { dirname, resolve } from 'node:path'; +import { describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import type { Pool } from 'pg'; + +import { runCoreMigrations } from '../src/platform/coreMigrations.js'; + +const here = dirname(fileURLToPath(import.meta.url)); +const srcDir = resolve(here, '..', 'src'); + +/** A pool that only has to be closable — no query ever reaches it, because + * every case here injects `runMigrations`. Cast because `pg.Pool` has a large + * surface and this test needs exactly one method of it. */ +function fakePool(onEnd: () => void): Pool { + return { end: async (): Promise => { onEnd(); } } as unknown as Pool; +} + +describe('#796 boot wiring — core migrations precede plugin activation', () => { + it('calls runCoreMigrations before toolPluginRuntime.activateAllInstalled()', async () => { + const src = await readFile(resolve(srcDir, 'index.ts'), 'utf8'); + + const callIdx = src.indexOf('await runCoreMigrations('); + assert.notEqual( + callIdx, + -1, + 'index.ts no longer awaits runCoreMigrations — core would boot without its ' + + 'own schema again (#796)', + ); + + const activateIdx = src.indexOf('await toolPluginRuntime.activateAllInstalled()'); + assert.notEqual(activateIdx, -1, 'could not find activateAllInstalled() in index.ts'); + + assert.ok( + callIdx < activateIdx, + 'core migrations must run BEFORE activateAllInstalled(): ToolPluginRuntime ' + + "reads a plugin's SQL-grant row while building its context, so " + + 'plugin_sql_grants has to exist by then', + ); + }); + + it('does not gate the boot call on any provider credential', async () => { + const src = await readFile(resolve(srcDir, 'index.ts'), 'utf8'); + const callIdx = src.indexOf('await runCoreMigrations('); + assert.notEqual(callIdx, -1); + + // The statement and the comment block introducing it. A provider key named + // anywhere in that window means the ledger was re-attached to a credential, + // which is precisely the #796 defect. + const window = src.slice(Math.max(0, callIdx - 1_200), callIdx + 400); + for (const key of ['ANTHROPIC_API_KEY', 'OPENAI_API_KEY', 'resolveLlmProvider']) { + assert.ok( + !new RegExp(`${key}\\s*[)\\]}]?\\s*(&&|\\|\\||\\?|\\))`).test(window), + `core migrations appear to be conditional on ${key} again (#796)`, + ); + } + }); +}); + +describe('#796 boot wiring — lock contention must not kill the process', () => { + it('retries while another replica holds the migration lock, then succeeds', async () => { + // Verbatim shape of the migrator's timeout (registry/migrator.ts), which is + // what `isLockContentionError` keys on. + const contention = new Error( + '[multi-orchestrator] timed out after 2000ms waiting for the ' + + '_multi_orchestrator_migrations advisory lock; 47 migration(s) still ' + + 'pending (0001_init.sql) — another replica is mid-migration, retry the boot', + ); + + let attempts = 0; + let ended = 0; + const outcome = await runCoreMigrations({ + databaseUrl: 'postgres://unused/db', + createPool: () => fakePool(() => { ended += 1; }), + runMigrations: async () => { + attempts += 1; + if (attempts < 3) throw contention; + }, + }); + + assert.equal(outcome, 'applied'); + assert.equal(attempts, 3, 'contention should be retried, not propagated to main()'); + assert.equal(ended, 1, 'the boot pool must still be closed exactly once'); + }); + + it('propagates a real migration failure on the first attempt', async () => { + let attempts = 0; + let ended = 0; + const boom = new Error('syntax error at or near "CREATE" in 0042_thing.sql'); + + await assert.rejects( + runCoreMigrations({ + databaseUrl: 'postgres://unused/db', + createPool: () => fakePool(() => { ended += 1; }), + runMigrations: async () => { attempts += 1; throw boom; }, + }), + /syntax error/, + ); + + assert.equal(attempts, 1, 'a broken migration must fail loudly, not be retried'); + assert.equal(ended, 1, 'the pool must be closed on the failure path too'); + }); +}); + +describe('#796 the retry predicate is pinned to the migrator that produces it', () => { + it('migrator.ts still emits the phrase isLockContentionError matches', async () => { + const migrator = await readFile( + resolve( + srcDir, + '..', + 'packages', + 'harness-orchestrator', + 'src', + 'registry', + 'migrator.ts', + ), + 'utf8', + ); + const core = await readFile(resolve(srcDir, 'platform', 'coreMigrations.ts'), 'utf8'); + + const phrase = 'waiting for the ${LOCK_KEY} advisory lock'; + assert.ok( + migrator.includes(phrase), + 'migrator.ts no longer builds its timeout message from ' + + `\`${phrase}\` — coreMigrations.isLockContentionError would stop ` + + 'recognising contention and boot would crash on a replica race instead ' + + 'of waiting. Update both together.', + ); + assert.ok( + migrator.includes("const LOCK_KEY = '_multi_orchestrator_migrations';"), + 'LOCK_KEY changed value — the phrase coreMigrations.ts matches is built from it', + ); + assert.ok( + core.includes("'waiting for the _multi_orchestrator_migrations advisory lock'"), + 'coreMigrations.ts no longer matches the migrator phrase this test pins', + ); + }); +});