fix(mcp): contribute Connect an Agent into the Account app nav so a non-admin can mint their own key

.changeset/16746-connect-agent-account-nav.md

@@ -0,0 +1,50 @@
+---
+'@objectstack/mcp': patch
+---
+
+Connect an Agent is reachable from the Account app, so a non-admin can mint their own key
+
+`POST /api/v1/keys` mints a `sys_api_key` bound to the **caller**, and the
+Connect-an-Agent page says the key "acts as you". But the page's only navigation
+entry sat in the Setup app, which declares `requiredPermissions:
+['setup.access']` — so every non-admin following the shipped two-step guide, and
+every reader of the runtime's own error text (`packages/mcp/src/plugin.ts`:
+*"mint an API key (Setup → Connect an Agent, or POST /api/v1/keys)"*, and
+`README.md`), stopped at step 1 while the endpoint behind the button had accepted
+them all along. Measured before: a principal with no system permissions gets
+`403 PERMISSION_DENIED` on `GET /api/v1/meta/apps/setup` and `nav_connect_agent`
+is absent from the wire.
+
+`CONNECT_AGENT_UI_BUNDLE` now carries a **second** `navigationContributions`
+entry, targeting the `account` app's `grp_account_developer` group beside the
+`nav_account_api_keys` entry already shipping there. Measured after, over the
+real composition (real `SETUP_APP` / `ACCOUNT_APP` / `SETUP_NAV_CONTRIBUTIONS`,
+the real fold and the real RBAC-by-route filter): the same permissionless
+principal gets `200` on `GET /api/v1/meta/apps/account` with
+`grp_account_developer` carrying `['nav_account_api_keys',
+'nav_account_oauth_apps', 'nav_connect_agent']`, while `apps/setup` still
+answers `403 PERMISSION_DENIED` with `connect_agent` absent from that body.
+
+**Nothing else moves.** No backend change, no authorization change, no change to
+which permissions exist, and the published "acts as you" promise is unchanged —
+it simply becomes keepable for the users it was written for. The Setup entry
+stays exactly as it was, so admins keep the page where the guide points, and no
+gate is added or removed anywhere: a navigation contribution registers exactly
+when the page registers, so an opted-out deployment
+(`OS_MCP_SERVER_ENABLED=false`) still gets no page and neither entry.
+
+⛔ Ungating Setup was **not** the fix, and was measured rather than assumed: the
+app-level `setup.access` gate fires before the group gate, so dropping the group
+gate alone changes nothing, and dropping both serves 14+ unrelated Setup
+surfaces (Users, Organization, Business Units, Branding, Feature Flags, …) to
+every signed-in user. ⛔ Nor was a `requiresService: 'mcp'` gate on an
+`account.app.ts` entry: the `mcp` service registers unconditionally in `init()`
+while this bundle registers behind `isMcpServerEnabled()`, so such an entry
+would outlive its page and 404 for every signed-in user on an opted-out
+deployment.
+
+Both entries deliberately share the item id `nav_connect_agent` — one
+destination, one identity. That is scoped, not a collision: `SchemaRegistry`
+keys contributions by target app and `applyNavContributions(app)` consults only
+that app's bucket, so a nav item id is unique within one app's navigation tree,
+and the translation bundles are keyed `apps.<app>.navigation.<id>`.

packages/mcp/src/connect-agent-account-nav.test.ts

@@ -0,0 +1,171 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+//
+// #16746 — the Connect-an-Agent page must be reachable by the principal the
+// page's own promise is about.
+//
+// `POST /api/v1/keys` mints a `sys_api_key` bound to the CALLER and the page
+// says the key "acts as you". Reached only through Setup it could not keep that
+// promise: `SETUP_APP` declares `requiredPermissions: ['setup.access']`, so
+// every non-admin following the shipped two-step guide — and the runtime's own
+// error text (`plugin.ts`, `README.md`: "mint a key in Setup → Connect an
+// Agent") — stopped at step 1 while the endpoint behind the button accepted
+// them all along. Maintainer ruling 2026-09-08, option A: open the key card to
+// every signed-in user; backend, authorization and the published promise do
+// not move.
+//
+// ---------------------------------------------------------------------------
+// What this file pins, and what it deliberately does NOT claim
+// ---------------------------------------------------------------------------
+// ⚠️ The acceptance criterion is a WIRE fact about two apps — a permissionless
+// principal sees `nav_connect_agent` in the `account` app's nav while
+// `GET /api/v1/meta/apps/setup` keeps answering 403 PERMISSION_DENIED — and
+// that fact cannot be measured from this package: `@objectstack/mcp` declares
+// no dependency on `@objectstack/rest` (the RBAC-by-route harness), on
+// `@objectstack/objectql` (`SchemaRegistry.applyNavContributions`, the fold) or
+// on `@objectstack/platform-objects` (`SETUP_APP` / `ACCOUNT_APP`). ⛔ So this
+// file does not reimplement any of them — a second copy of the fold is exactly
+// the divergence `cli/src/utils/nav-contribution-groups.ts` refuses to write.
+//
+// What it pins instead is the half this package OWNS, stated as the two
+// properties the wire fact rests on:
+//
+//   1. the contribution is aimed at the ungated app and group, and carries
+//      nothing the server-side nav filter could strip for a permissionless
+//      caller (`requiredPermissions` / `requiresService`);
+//   2. this bundle cannot widen Setup — the accident that would make a
+//      "the entry is visible" assertion pass for the wrong reason.
+//
+// The second is the load-bearing one. Ungating Setup was measured on the real
+// composition and refused: the app-level `setup.access` gate fires BEFORE the
+// group gate (so dropping `group_integrations`' gate alone changes nothing),
+// and dropping both serves 14+ unrelated Setup surfaces to every signed-in
+// user. Nothing in this bundle may reintroduce that, so the walk below asserts
+// the bundle declares no permission key anywhere and no `apps` collection that
+// could redeclare `SETUP_APP` without its gate.
+
+import { describe, it, expect } from 'vitest';
+import { NavigationContributionSchema } from '@objectstack/spec/ui';
+
+import { CONNECT_AGENT_UI_BUNDLE } from './connect-ui.js';
+
+type AnyRec = Record<string, unknown>;
+
+const contributions = CONNECT_AGENT_UI_BUNDLE.navigationContributions as AnyRec[];
+const byApp = (app: string): AnyRec[] => contributions.filter((c) => c.app === app);
+const itemsOf = (c: AnyRec): AnyRec[] => (c.items ?? []) as AnyRec[];
+
+/** Every key present anywhere in a value, depth-first — objects and arrays. */
+function everyKey(value: unknown, out: string[] = []): string[] {
+  if (Array.isArray(value)) {
+    for (const entry of value) everyKey(entry, out);
+  } else if (value && typeof value === 'object') {
+    for (const [key, child] of Object.entries(value)) {
+      out.push(key);
+      everyKey(child, out);
+    }
+  }
+  return out;
+}
+
+describe('#16746 — Connect an Agent reaches the per-user Account app', () => {
+  it('contributes into the `account` app, into the group that already ships API Keys', () => {
+    const account = byApp('account');
+    expect(account).toHaveLength(1);
+    // `ACCOUNT_APP` declares no `requiredPermissions` (deliberately: every
+    // authenticated user must reach their own security surface, RLS scopes the
+    // rows), and `grp_account_developer` is the group already carrying
+    // `nav_account_api_keys`. Both are read by NAME here — this card edits
+    // nothing in `@objectstack/platform-objects`.
+    expect(account[0]).toMatchObject({ app: 'account', group: 'grp_account_developer' });
+  });
+
+  it('aims one `page` item at `connect_agent`, the page this same bundle registers', () => {
+    const items = itemsOf(byApp('account')[0]);
+    expect(items).toHaveLength(1);
+    expect(items[0]).toMatchObject({
+      id: 'nav_connect_agent',
+      type: 'page',
+      pageName: 'connect_agent',
+    });
+    // The destination must be a page this bundle actually ships, or the entry
+    // is a 404-when-clicked shown to every signed-in user — the precise defect
+    // that ruled out gating an `account.app.ts` entry on `requiresService:
+    // 'mcp'` (the service registers unconditionally in `init()`; this bundle
+    // registers behind `isMcpServerEnabled()`).
+    const pageNames = (CONNECT_AGENT_UI_BUNDLE.pages ?? []).map((p) => p.name);
+    expect(pageNames).toContain(items[0].pageName);
+  });
+
+  it('carries nothing the per-user caller could be filtered on', () => {
+    // THE criterion, stated at the layer this package owns. The server-side
+    // nav filter strips an item for a caller missing its `requiredPermissions`
+    // and for an absent `requiresService` capability. An entry carrying either
+    // would be invisible to exactly the principal this card is about — a
+    // permissionless one — while every assertion above still passed.
+    const item = itemsOf(byApp('account')[0])[0];
+    expect(item).not.toHaveProperty('requiredPermissions');
+    expect(item).not.toHaveProperty('requiresService');
+    expect(item).not.toHaveProperty('visible');
+  });
+
+  it('leaves the Setup entry exactly as it was — admins keep the page where the guide points', () => {
+    const setup = byApp('setup');
+    expect(setup).toHaveLength(1);
+    expect(setup[0]).toMatchObject({ app: 'setup', group: 'group_integrations', priority: 110 });
+    expect(itemsOf(setup[0])).toEqual([
+      { id: 'nav_connect_agent', type: 'page', pageName: 'connect_agent', label: 'Connect an Agent', icon: 'bot' },
+    ]);
+  });
+
+  it('⛔ cannot widen Setup — no permission key and no app redeclaration anywhere in the bundle', () => {
+    // The risk the green has to prove, not merely pass. A test asserting only
+    // "the account entry exists" would go green just as happily on a diff that
+    // reached the card by ungating Setup instead — measured to serve 14+
+    // unrelated Setup surfaces (Users, Organization, Branding, Feature Flags,
+    // …) to every signed-in user. This bundle is the one file that changed, so
+    // it is where that accident would have to be written.
+    const keys = new Set(everyKey(JSON.parse(JSON.stringify(CONNECT_AGENT_UI_BUNDLE))));
+    expect([...keys].filter((k) => k === 'requiredPermissions')).toEqual([]);
+    // An `apps: [...]` collection here could redeclare `SETUP_APP` — last
+    // registration wins — and drop its `setup.access` gate without touching
+    // `platform-objects` at all.
+    expect(CONNECT_AGENT_UI_BUNDLE).not.toHaveProperty('apps');
+    // Every contribution aims at a named group. A contribution with no `group`
+    // appends at the app's TOP level, which for Setup would put the entry
+    // outside `group_integrations`' gate.
+    for (const c of contributions) expect(typeof c.group).toBe('string');
+  });
+
+  it('shares the item id across the two apps, and the fold says that is scoped per app', () => {
+    // Answered from the fold rather than from taste, because a wrong answer
+    // here is a silent one. `SchemaRegistry` keys contributions by TARGET APP
+    // (`appNavContributions: Map<string, …>`) and `applyNavContributions(app)`
+    // consults only `get(app.name)`, so a nav item id is unique within ONE
+    // app's navigation tree; nothing indexes it across apps (no id-keyed
+    // registry, no de-duplication by id), and the translation bundles are
+    // keyed `apps.<app>.navigation.<id>`, which makes one shared id two
+    // distinct keys. One destination therefore keeps one identity.
+    const targets = contributions.map((c) => c.app);
+    expect(new Set(targets).size).toBe(targets.length);
+    expect(contributions.flatMap((c) => itemsOf(c).map((i) => i.id))).toEqual([
+      'nav_connect_agent',
+      'nav_connect_agent',
+    ]);
+    // ⛔ …and they are two literals, not one shared const. The fold
+    // `structuredClone`s the APP but pushes `...c.items` BY REFERENCE, so one
+    // shared object would sit in two apps' navigation trees at once and any
+    // in-place consumer edit would leak from one app into the other.
+    expect(itemsOf(byApp('setup')[0])[0]).not.toBe(itemsOf(byApp('account')[0])[0]);
+  });
+
+  it('both contributions parse against the real spec contract', () => {
+    // The shared id is not merely unenforced — it is accepted by the schema
+    // that governs the surface, checked against the spec rather than asserted
+    // about it.
+    for (const c of contributions) {
+      const parsed = NavigationContributionSchema.safeParse(c);
+      expect(parsed.error?.issues ?? []).toEqual([]);
+      expect(parsed.success).toBe(true);
+    }
+  });
+});

packages/mcp/src/connect-ui.ts

@@ -1,7 +1,7 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 /**
- * "Connect an agent" Setup page — plugin-carried UI metadata (#2714 Phase 1,
+ * "Connect an agent" page — plugin-carried UI metadata (#2714 Phase 1,
  * objectui#2363).
  *
  * The page ships WITH the MCP capability (same principle as the marketplace
@@ -16,6 +16,29 @@
  * Registered by {@link MCPServerPlugin} on `kernel:ready`, gated on the same
  * default-on switch as the HTTP surface — an opted-out deployment
  * (`OS_MCP_SERVER_ENABLED=false`) gets no page and no nav entry.
+ *
+ * ## Two nav entry points, one page (#16746)
+ *
+ * `POST /api/v1/keys` mints a `sys_api_key` bound to the CALLER, and this page
+ * says the key "acts as you" — so the surface that mints it is a per-user
+ * credential surface, not an administrative one. Reached only through Setup it
+ * could not keep that promise: Setup declares `requiredPermissions:
+ * ['setup.access']`, so every non-admin following the two-step guide stopped at
+ * step 1 while the endpoint behind the button accepted them all along.
+ *
+ * The fix is a SECOND contribution, into the `account` app's Developer group
+ * beside the `nav_account_api_keys` entry already shipping there. The Setup
+ * entry stays for admins. ⛔ Ungating Setup is NOT the fix and was measured:
+ * the app-level `setup.access` gate fires BEFORE the group gate, so dropping
+ * the group gate alone changes nothing, and dropping both serves 14+ unrelated
+ * Setup surfaces (Users, Organization, Branding, Feature Flags, …) to every
+ * signed-in user — far past what this card asks for. ⛔ Nor is a
+ * `requiresService: 'mcp'` gate on an `account.app.ts` entry: the `mcp` service
+ * registers unconditionally in `init()` while this bundle registers behind
+ * `isMcpServerEnabled()`, so such an entry outlives its page and 404s on an
+ * opted-out deployment. A contribution registers exactly when the page
+ * registers, which is why both entries live in THIS bundle and neither carries
+ * a gate of its own.
  */
 
 import type { Page } from '@objectstack/spec/ui';
@@ -59,8 +82,22 @@ export const CONNECT_AGENT_UI_BUNDLE = {
   type: 'plugin',
   scope: 'system',
   name: 'Connect an Agent UI',
-  description: 'Setup page + navigation for connecting MCP clients to this environment.',
+  description: 'Connect-an-Agent page + Setup and Account navigation for connecting MCP clients to this environment.',
   pages: [CONNECT_AGENT_PAGE],
+  // Both entries point at the one `connect_agent` page and share the item id.
+  // That is legal and deliberate, read off the fold rather than assumed: the
+  // registry keys contributions by TARGET APP
+  // (`appNavContributions: Map<string, …>`) and `applyNavContributions(app)`
+  // consults only `get(app.name)`, so a nav item id is unique within one app's
+  // navigation tree and nothing indexes it across apps — no id-keyed registry,
+  // no de-duplication by id, and the translation bundles are keyed
+  // `apps.<app>.navigation.<id>` (per-app namespaces, so one id yields two
+  // distinct keys). Sharing it keeps ONE identity for one destination.
+  //
+  // ⛔ The two items are separate object literals, not one shared const: the
+  // fold `structuredClone`s the APP but pushes `...c.items` by reference, so a
+  // shared literal would put the same object in two apps' navigation trees and
+  // any in-place consumer edit would leak across them.
   navigationContributions: [
     {
       app: 'setup',
@@ -76,5 +113,29 @@ export const CONNECT_AGENT_UI_BUNDLE = {
         },
       ],
     },
+    {
+      // The per-user half. `ACCOUNT_APP` declares no `requiredPermissions` —
+      // deliberately, so every authenticated user reaches their own security
+      // surface — and `grp_account_developer` already carries
+      // `nav_account_api_keys`. Targeted by NAME; `packages/platform-objects`
+      // is not edited.
+      app: 'account',
+      group: 'grp_account_developer',
+      // `priority` orders contributions among THEMSELVES within the group, and
+      // the group's own static children always precede them. Stated rather
+      // than defaulted, at the schema's declared default: no other package
+      // contributes into this group today, so there is nothing to interleave
+      // with and no reason to claim a position.
+      priority: 200,
+      items: [
+        {
+          id: 'nav_connect_agent',
+          type: 'page',
+          pageName: 'connect_agent',
+          label: 'Connect an Agent',
+          icon: 'bot',
+        },
+      ],
+    },
   ],
 };