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 b798cab6..add4b913 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'; @@ -1707,6 +1708,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..07ac4389 --- /dev/null +++ b/middleware/src/platform/coreMigrations.ts @@ -0,0 +1,179 @@ +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. + * + * 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 + * 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; + /** + * 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( + 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 runMigrations = opts.runMigrations ?? runMultiOrchestratorMigrations; + + const pool = createPool(databaseUrl); + const deadline = Date.now() + LOCK_CONTENTION_TOTAL_WAIT_MS; + try { + 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. + 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 bca20e0b..81e5b285 100644 --- a/middleware/src/plugins/manifestLoader.ts +++ b/middleware/src/plugins/manifestLoader.ts @@ -398,6 +398,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; @@ -505,6 +510,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 }; } @@ -526,17 +539,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/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', + ); + }); +}); 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 } : {}),