feat(connectors): unified Connectors UX — registries + permissions + operator setup#166
Merged
Conversation
mgoldsborough
force-pushed
the
feat/interactive-oauth
branch
2 times, most recently
from
15ce2a0 to
69a2ded
Compare
Track B foundation. Adds the schema field that drives per-member credential scoping and routes provider persistence to the right directory based on whose tokens are being read/written. - src/bundles/types.ts: BundleRef.url variant gains optional oauthScope: 'workspace' | 'member' (default 'workspace'). Documents the per-bundle meaning + the opt-in-per-member semantic for member-scope. - src/tools/workspace-oauth-provider.ts: constructor accepts optional memberId. Validated at construction with a tight regex (alphanumerics, ._-, length 1-128, no '..' / '.', no path separators) so member ids can never traverse out of the credentials tree. Splits the persistence dir into two paths: clientDir (workspace-shared, for client.json — DCR registration represents NimbleBrain-as-a-client per workspace, not per member) and tokenDir (per-member when memberId is set, workspace-level otherwise — for tokens.json, verifier.json, and Track B's identity.json). invalidateCredentials and the file I/O helpers parameterized by dir. Adds getMemberId() so downstream callers (disconnect route, snapshot) can key per-principal records. - src/runtime/workspace-runtime.ts: member-scope URL bundles skip startBundleSource at boot — there's no member identity to authenticate as. Instead the bundle is registered with metadata only, and Connections are created on-demand when a member calls a tool or hits Connect. - src/bundles/lifecycle.ts: seedInstance branches on oauthScope. Workspace-scope keeps existing behavior (auto-Connection at _workspace). Member-scope seeds with empty connections map and instance.state = 'stopped' until first member connects. - Tests: 6 new member-scope provider tests covering memberId validation, per-member token isolation, workspace-shared client.json, scoped invalidate, and the getMemberId getter. Total 12 unit tests on the provider, all pass.
…ource
ToolSource.execute and ToolRouter.execute gain optional principalId so
member-scoped MCP bundles can route tool calls to the calling member's
authenticated source. Workspace-scoped sources (existing McpSource,
UpjackSource, in-process) ignore the parameter — only MemberPoolSource
reads it.
MemberPoolSource is the new abstraction for oauthScope='member' URL
bundles: one registry entry per bundle, internal Map<principalId,
McpSource>. Each per-member McpSource gets its own Client/Transport/
auth provider, so concurrent calls from different members never contend
on shared mutable state and refresh-token expiry isolates per principal.
- Tool list cached at the pool layer; first connecting member fills it.
Until any member connects, tools() returns [] (catalog page surfaces
'Connect to access N tools' as the affordance).
- Execute returns structured pending_auth error when called for an
unconnected principal, including {code, serverName, principalId} in
structuredContent so the agent can surface a Connect prompt.
- setMemberSource replaces an existing entry (stops the old source).
removeMember stops + clears. stop() tears down everyone.
10 unit tests cover dispatch, isolation, cache, replace, remove, stop.
Adversarial review notes (see REMOTE_MCP_CONNECTIONS.md design § 4):
- Concurrency: separate Clients per principal — no shared mutable state.
- Failure isolation: per-member transport state.
- Tool-list staleness: cache served from any active member; refreshed on
next miss after stop().
- Forge prevention: principalId set at API layer from authenticated
identity; agent loop doesn't choose it.
…e registration
Member-scoped URL bundles get their MemberPoolSource registered in the
per-workspace ToolRegistry at seed time so the bundle is visible to the
agent immediately (with empty tools() until first member connects). The
pool reference also lives in lifecycle.memberPools keyed by
`serverName|wsId` so startMemberAuth can find it.
- BundleInstance gains optional `ref?: BundleRef` so lifecycle has
the URL + transport config + (Track A) oauthClient available when
constructing per-member sources on-demand. Stored as an opaque copy
in seedInstance.
- seedInstance accepts an optional `registry: ToolRegistry` so it can
register the MemberPoolSource directly. Runtime threads the per-
workspace registry through after constructing it.
- lifecycle.startMemberAuth(serverName, wsId, principalId, opts):
constructs a per-member WorkspaceOAuthProvider (memberId set), creates
an McpSource, adds it to the pool, kicks off start() in the
background, and races the provider's onInteractiveAuthRequired
callback against a 15s timeout. Returns the captured authorization
URL. Idempotent for double-clicks (reuses existing pending_auth URL).
- /v1/mcp-auth/initiate: when no existing pending_auth Connection
exists AND the bundle is member-scope AND the principal is a real
member id, calls startMemberAuth to bootstrap the flow. Returns the
same { authorizationUrl } shape so the web banner code is unchanged.
- Runtime gains getAllowInsecureRemotes() so /initiate can pass the
SSRF allowlist setting to startMemberAuth's provider construction.
2360 unit tests + 19 OAuth integration tests pass.
…tity rule ToolRouter.execute and ToolSource.execute already accept principalId. This wires the identity through the two execution paths: - POST /v1/tools/call (REST): handlers.ts passes the authenticated identity's id as principalId. Member-scoped bundles route to the caller's per-principal source; workspace-scoped sources ignore it. - Engine (chat / agent loop): EngineConfig gains an optional principalId field, threaded into engine.execute()'s registry call. runtime.ts populates it from request.identity (which the chat handler already sets from the API middleware) so the agent-loop identity rule from the spec is satisfied: 'agent-loop calls inside a chat owned by member A use A's tokens.' This closes the per-call routing gap left by a34f5da. Member-scoped bundles called from a chat owned by member B will: - find member B's per-principal McpSource via MemberPoolSource - if absent or pending_auth: return structured pending_auth error so the agent surfaces 'connect this bundle' to the user - if running: use B's tokens for the upstream request
WorkspaceOAuthProvider.revokeAndDeleteTokens(opts) does best-effort
revocation against the upstream AS, then always deletes local files
regardless of upstream result. Local file deletion alone leaves stale
refresh tokens valid until natural expiry — this closes the gap.
- Discovers revocation_endpoint via OAuth metadata (RFC 8414) at the
bundle URL's origin.
- POST x-www-form-urlencoded with token + token_type_hint + client_id
(+ client_secret when the registration carries one). Refresh token
revoked first when present (RT is the longer-lived credential; if
access-token revocation races, the AS won't issue a fresh AT once
the RT is gone).
- RFC 7009 § 2.2 handling: 400 invalid_token treated as success
(token already invalid → goal already met).
- SSRF defense applied to the metadata discovery URL using the same
allowInsecureRemotes flag that gates bundle URLs.
- Best-effort discipline: discovery failures, network errors, or
non-2xx responses log and continue to local cleanup. Returns a
structured { revoked, deletedLocal, error? } so callers can log
partial success.
- 4 new unit tests cover: no-tokens fast-path, full revoke + delete,
discovery failure → local-only, RFC 7009 invalid_token treated as
success.
Track C will call this from the disconnect route; lifecycle will call
it on workspace member removal + bundle uninstall.
When the AS returns an id_token alongside the OAuth tokens (Google,
Microsoft, Zoom, plus most OAuth 2.1 servers), parse the JWT payload
and stash { sub, email, name } at identity.json (mode 0o600 via the
same atomic-write helper as tokens). Powers 'Connected as
mat@nimblebrain.ai' on the Connections page without an extra
round-trip to a userinfo endpoint.
- parseIdTokenClaims: base64url-decodes the JWT payload, extracts the
three claims we surface, returns null on any parse failure. NO
signature verification — the SDK fetched the token from the AS over
TLS, which is the trust anchor we already rely on for the
access_token; we treat id_token claims as informational only and
never make access decisions on them.
- WorkspaceOAuthProvider.saveTokens: opportunistically calls the
parser when id_token is present. Failures are logged at debug and
swallowed — auth still succeeds; the UI just doesn't get a display
name.
- WorkspaceOAuthProvider.identity(): reader for the Connections page.
Returns null when no id_token was issued.
- invalidateCredentials('tokens') now also removes identity.json
(1:1 with tokens — when tokens go, the identity stops being
meaningful).
- 4 new tests cover capture, no-id_token case, invalidate cleanup,
and malformed id_token graceful handling.
Closes Track B foundation. Tracks A + C next, then verify + Chrome
E2E + PR.
…rizationParams
Unlocks vendors that don't support DCR — HubSpot, Asana, Gmail, Outlook,
Zoom Marketplace user-OAuth apps. Operator pre-registers an app in the
vendor's developer portal; bundle entry references the credentials.
Schema (BundleRef.url variant + new OAuthClientConfig type):
- oauthClient: { clientId, clientSecret: { ref: 'credential', key },
tokenEndpointAuthMethod? }
- scopes: string[]
- additionalAuthorizationParams: Record<string, string>
Provider (WorkspaceOAuthProvider):
- New constructor opts: staticClient (resolved client + secret),
scopes, additionalAuthorizationParams.
- clientInformation(): returns the static client when present,
bypassing DCR persistence entirely.
- saveClientInformation(): no-op when staticClient is set so a stray
SDK call can't overwrite the static config to disk.
- clientMetadata.scope: scopes joined with single-space per RFC 6749.
- clientMetadata.token_endpoint_auth_method: 'client_secret_post'
when secret present without explicit override; 'none' for DCR /
PKCE-only; explicit override always wins.
- redirectToAuthorization(): appends additionalAuthorizationParams
to the URL before the redirect probe (so Google's access_type=offline
+ prompt=consent reach the AS).
Reserved-key allowlist (RESERVED_AUTHORIZE_PARAMS):
- client_id, redirect_uri, response_type, state, code_challenge,
code_challenge_method, scope. Throws at construction AND at
seedInstance time. A misconfigured catalog entry can't override
PKCE binding or redirect target.
Boot wiring (startup.ts + lifecycle.startMemberAuth):
- Both paths now resolve oauthClient.clientSecret via FileCredentialStore
(workspace-scoped) and pass the resolved staticClient to the provider.
- Both paths thread scopes + additionalAuthorizationParams through.
- Missing credential → bundle install fails with a clear 'run nb
credential set <wsId> <key> <value>' message.
7 new unit tests (27 total): static-client clientInformation; static-
client save no-op + no client.json on disk; scope serialization;
default token-auth-method for DCR/static; explicit override; reserved-
key rejection; URL-param appending verified by spied fetch.
`nb credential` (alias `nb creds`) — set/get/delete/list workspace- scoped opaque secrets backed by FileCredentialStore. The set/get/delete operations go through the same boundary the SaaS-encrypted store will swap; list is operator-only diagnostic UX (direct readdir) and stays out of the runtime contract. nb credential set <wsId> <key> <value> # write atomically nb credential get <wsId> <key> # print to stdout, no decoration nb credential delete <wsId> <key> # idempotent nb credential list <wsId> # keys only, never values Designed for the operator UX of seeding oauthClient.clientSecret references in workspace.json: nb credential set ws_mat zoom.client_secret $ZOOM_SECRET Track C's web admin modal exposes the same primitive behind an auth-gated route (see Track C connections routes). Smoke-tested end-to-end against a temp work dir.
…nect) Catalog distribution per spec § Catalog distribution: - src/connections/catalog.ts — DEFAULT_CONNECTION_CATALOG with 7 entries (Asana, Notion org, Gmail, Granola, HubSpot, Outlook, Zoom). Each entry: id (kebab), name, description, iconUrl (static.nimblebrain.ai), url, auth (dcr|static), defaultScope, optional requiredScopes / additionalAuthorizationParams / operatorSetup / tags. Secrets stay out — static-auth entries reference the credential store via operatorSetup.credentialKey. - src/connections/load-catalog.ts — loadCatalog() reads NB_CATALOG_PATH env override (full replace, not merge — see decision rationale in spec). validateCatalog() drops malformed entries with logged warnings, rejects duplicate ids, enforces static→operatorSetup invariant. Top-level array required; falls back to default catalog on parse / shape errors. - workspace.connectionsAllowList: optional string[] on Workspace; when set, only listed catalog ids appear on that workspace's Connections page. Filter is for catalog visibility only — installed bundles continue to function regardless. Routes (src/api/routes/connections.ts): - GET /v1/connections/catalog — workspace-authed; returns catalog filtered by allow-list when present. - GET /v1/connections/installed — workspace-authed; joins workspace.bundles (URL-only) + catalog metadata + lifecycle's per-principal connection state + credential-store probe for static- auth bundles. Returns myConnection (caller's per-principal state) for member-scope and workspaceConnection for workspace-scope. Includes OIDC identity (sub/email/name from identity.json) when available. - POST /v1/connections/disconnect — workspace-authed; calls WorkspaceOAuthProvider.revokeAndDeleteTokens (RFC 7009 best-effort revoke + always-delete-locally), removes the per-member entry from MemberPoolSource, and emits connection.state_changed=stopped so the UI clears immediately. 8 catalog tests (validation rules, dup-id rejection, optional-field shape coercion). All 2383 unit tests pass; OAuth integration tests unaffected.
Settings → Connections page renders catalog + installed state in two
columns (My Connections | Workspace Connections), plus a Custom URL
Bundles section for orphans.
Per-card UX:
- Status pill: not_installed / not_connected / pending_auth / starting
/ running ('Connected as <email>' when OIDC identity is captured)
/ dead / crashed.
- 'Connect' button: POSTs /v1/mcp-auth/initiate then window.location
.assign(authorizationUrl) — same flow the pending-auth banner uses.
- 'Disconnect' button (when running): POST /v1/connections/disconnect
→ revokes upstream + clears local + removes from MemberPoolSource.
- 'Operator setup required' label (admin) when static-auth bundle has
no client_secret seeded — surfaces the credential key the operator
needs to set via `nb credential set`.
Web client (web/src/api/client.ts):
- ConnectionCatalogEntry / InstalledConnection types mirror server.
- getConnectionsCatalog / getInstalledConnections / disconnectConnection
helpers. All go through the existing request<T>() with credentials.
App.tsx + SettingsPage.tsx:
- Mounts /settings/workspace/connections → ConnectionsTab.
- Adds 'Connections' to the workspace settings sidebar between Apps
and Skills.
Web build clean. Type check clean. Unit tests unchanged from prior
commits (1 pre-existing unhandled-rejection error from DOCX-extract
test, present before this branch — not a regression).
Auto-fixes from `bun run format` + organizeImports. No behavior changes.
W1 — OIDC UTF-8 decode (workspace-oauth-provider.ts parseIdTokenClaims):
atob returns a Latin-1 binary string and was mangling multibyte names
(e.g. '山田太郎'). Decode through Uint8Array + TextDecoder so JSON.parse
sees the bytes the AS actually sent.
W2 — Disconnect leaks stale source ref (api/routes/connections.ts):
recordConnectionStateChange('stopped') was inheriting the existing
source field (still pointing at the now-stopped McpSource). Pass
{ source: null } explicitly so the Connection record drops the
reference.
W3 — Catalog reserved-key validation (connections/load-catalog.ts):
validateAdditionalAuthorizationParams now runs at catalog-load time,
not just provider construction. A misconfigured catalog ConfigMap
with reserved keys (client_id, state, code_challenge, etc.) fails
fast at boot with a clear per-entry warning instead of dying at
runtime OAuth-flow time.
W4 — iconUrl protocol allowlist (connections/load-catalog.ts):
isSafeIconUrl rejects javascript:/data:/file:/etc. iconUrl renders
as <img src> in the Connections page; React's JSX doesn't sanitize
that. data:image/svg+xml SVG vectors with embedded <script> are real.
Allowed: http(s) absolute URLs and '/'-relative paths.
W5 — RFC 9728 OAuth Protected Resource Metadata for revocation
(workspace-oauth-provider.ts revokeAndDeleteTokens):
Discovery now tries /.well-known/oauth-protected-resource first to
find authorization_servers[], then probes each AS's /.well-known/
oauth-authorization-server for revocation_endpoint. Falls back to the
bundle origin (covers Granola/Notion/HubSpot pattern). Without this,
revocation silently no-op'd for Google + Microsoft (AS at different
origin from bundle).
S6 — Add request/request_uri/response_mode to RESERVED_AUTHORIZE_PARAMS:
Defensive against OIDC-style param hijacking. response_mode in
particular could change response delivery (form_post, fragment) in
ways that break the callback.
S7 — Bound id_token input length: 16KB cap before parse to prevent
DoS via huge JWTs. Real JWT payloads run well under 4KB.
Tests: +3 cases (RFC 9728 cross-origin AS discovery, iconUrl
protocol allowlist, catalog-level reserved-key rejection); existing
catalog tests updated with valid iconUrl fixtures. Total 28 unit
tests on the provider, 10 on catalog validation. All pass.
…und path Step 1's POST /v1/mcp-auth/initiate route returned 404 'not_pending_auth' for any case where no pending flow existed. Track B added bundle existence checking up front, so non-existent bundles now return 404 'bundle_not_found' (and bundle existence + member-scope first-connect kicks off startMemberAuth). Update the existing test to assert against bundle_not_found, and extend StubLifecycle with a getInstance() method matching the new route surface.
mgoldsborough
force-pushed
the
feat/connections-tracks
branch
from
db6bb34 to
56d5842
Compare
Contributor
Author
|
Rebased onto main + retargeted. Step 1 (PR #175) is now in main, so this PR no longer carries those 7 commits. Branch was rebased with `git rebase --onto main 15ce2a0 feat/connections-tracks` — replays the 12 net-new Tracks A/B/C commits onto current main, dropping the 7 Step 1 commits as duplicates. One small follow-up commit on top: `56d5842` updates the `POST /v1/mcp-auth/initiate` 404 unit test to assert against the new `bundle_not_found` error code and extends `StubLifecycle` with the `getInstance()` method this PR's route handler now requires. Originally PR #175 tested only the legacy "no pending flow" 404 path; Track B added the bundle-existence check up front, so the test needed to follow. Local verification on the rebased branch:
|
WorkspaceOAuthProvider's interactive branch registers a flow then throws UnauthorizedError synchronously to propagate through the SDK. The registered Promise is consumed by McpSource.start() inside its retry path — but in tests (and any provider call path that catches the UnauthorizedError before the SDK awaits the deferred), no consumer attaches handlers. When the TTL fires later, or when test cleanup calls _clearAll(), the rejection has no handler and surfaces as an unhandled async error. Bun reports these between tests and exits 1, even with 0 test failures. Attach a defensive no-op .catch() to the registered promise. Real awaiters (McpSource, or tests that explicitly resolve/reject) still observe the settlement through their own handlers — multiple Promise handlers run independently. Surfaces in the full unit suite via cross-test ordering: workspace-oauth- provider.test.ts triggers redirectToAuthorization (which registers a flow then throws), oauth-flow-registry.test.ts beforeEach calls _clearAll() and rejects the orphan.
… + state refresh This is the self-service hot path closed end-to-end. Three structural changes that converge on a single design: # Architecture 1. Unified `lifecycle.startAuth(serverName, wsId, principalId, opts)` replaces `startMemberAuth`. Works for both workspace-scope and member-scope. Always tears down any stale source for the principal before constructing a fresh one — so disconnect → reconnect works without a process restart (the previous behavior left the McpSource alive in lifecycle's map with revoked tokens cached in memory; the route handler 404'd because it expected a pending flow that never existed). 2. `lifecycle.disconnect()` is a first-class method symmetric across scopes: revoke + delete tokens, tear down source, transition the Connection to `not_authenticated`. The disconnect route is now a thin shim. The 'workspace-scope teardown not yet wired' caveat from PR #166 is gone. 3. `POST /v1/connections/install` adds a catalog entry to `workspace.bundles[]` (atomic via WorkspaceStore.update) + seeds the lifecycle map. Idempotent. The Connections page Connect button calls /install before /initiate when the bundle isn't yet installed — one click, install + auth. # State machine refresh `BundleState` gains `not_authenticated` (initial state for URL bundles with no tokens, and the resting state after disconnect) and `reauth_required` (was running, refresh failed). `pending_auth` is narrowed to its true meaning: actively in an OAuth flow (user clicked Connect, awaiting callback). UI surface: - Status pill labels updated for the new states - 'Reconnect' button for reauth_required / dead / crashed - 'Connect' button for not_authenticated / not_installed / stopped # Banner removed The global PendingAuthBanner conflated 'never authenticated yet' with 'was working, broke.' Self-service install means a fresh bundle should NOT pop a banner — the user installed it but hasn't asked to authenticate yet. Reauth-needed surfaces inline on the Connections card (amber pill + Reconnect button); broken tool calls surface inline in chat via existing MCP error paths. No global blocking surface. Removed: web/src/components/PendingAuthBanner.tsx, the GET /v1/connections/pending route, lifecycle.getPendingConnections, and the App.tsx pending-auth state plumbing (~70 lines). # Boot path Workspace-scope URL bundles without persisted tokens skip source.start() at boot. `hasPersistedWorkspaceOAuthTokens` (new tiny helper in src/bundles/oauth-tokens.ts) is consulted in both workspace-runtime.ts (boot loop) and lifecycle.ts (seedInstance) so the no-token case never tries to start, and seedInstance lands the Connection in `not_authenticated`. RT-rejected at boot still goes through the pending-auth-buffer path — but seedInstance now records that as `reauth_required` instead of `pending_auth` so the UI surfaces the right affordance. # Verification - `bun run verify:static`: clean (1 pre-existing url-validator warning) - `bun run test:unit`: 2432 pass, 0 fail - `bun run test:integration`: 460 pass, 6 skip, 0 fail - `bun run smoke`: 17 pass, 0 fail
…sts + log on stop()
Adjudicated findings from /pr-review against the self-service refactor:
- /install rejects static-auth catalog entries with 409 'operator_setup_required'
before mutating workspace.json. v1 catalog schema only carries the
credentialKey for clientSecret, not clientId — installing static-auth would
persist a half-formed oauthClient that fails confusingly at startAuth time.
ConnectionsTab surfaces the affordance up front (button replaced with
'Operator setup required' + portal link) so the click → 409 round-trip
doesn't happen.
- /install header comment rewritten to reflect actual behaviour: any
authenticated workspace member can install. The catalog allow-list
(workspace.connectionsAllowList) is the gate that constrains what's
installable; uninstall (when added) will gate to admins.
- Disconnect route comment refreshed — the inline 'pool teardown' /
'pending_auth' specifics moved to lifecycle.disconnect; route is a thin
shim now.
- teardownConnectionSource logs on source.stop() failure instead of silent
swallow. Best-effort semantics preserved (we drop the source either way),
but stuck-source patterns are now visible in operator logs.
- Tests added:
- test/unit/lifecycle-startauth-disconnect.test.ts (10 tests):
rejection paths, scope-mismatch, debounce on existing pending_auth
URL, already-running rejection, disconnect transitions to
not_authenticated + clears source.
- test/unit/api/connections-install-route.test.ts (7 tests):
400 on bad body / missing catalogId, 404 on missing workspace /
catalog entry / not-in-allow-list, 409 on static-auth, happy path
with workspace.json mutation + lifecycle seed, idempotence on
duplicate install.
Verification:
- bun run verify:static — clean
- bun run test:unit — 2449 pass, 0 fail (17 new)
- bun run test:integration — 460 pass, 0 fail
- bun run smoke — 17 pass, 0 fail
…ing user to close tab
The OAuth flow navigates the user's existing tab to the AS and back to
/v1/mcp-auth/callback — they didn't open a new tab. Telling them to
'close this tab' is wrong (closes their browser entirely) and leaves
them stranded outside NimbleBrain.
Replace the success page with one that:
- Auto-redirects to /settings/workspace/connections after 800ms (JS
location.replace) + meta-refresh fallback (1s) for non-JS browsers
- Shows a 'Click here if you aren't redirected →' link (always-works
fallback for restrictive browsers / extensions)
- Resolves the return origin via NB_WEB_URL → NB_API_URL → request
origin. Single-origin production deployments work out of the box;
dev with split API/web ports works by setting NB_WEB_URL.
Existing test (assertion: html contains 'Authorization complete') still
passes — the heading text is unchanged.
The OAuth callback redirects users back to /settings/workspace/connections
on the web SPA. In dev, the API and SPA run on different ports — the
callback hits the API port (e.g. 27249) and we need to bounce the user
to the web port (e.g. 27248). Without NB_WEB_URL set, the callback's
fallback to NB_API_URL sends the user back to the API origin, which
returns a workspace_error JSON page instead of the Connections tab.
The 'bun dev' supervisor already knows NB_WEB_PORT (it spawns Vite
with it). Inject NB_WEB_URL=http://localhost:${NB_WEB_PORT} into the
API child's env when NB_WEB_PORT is set. Operator-supplied NB_WEB_URL
is preserved (the auto-derivation only fills in the unset case).
Production deployments are unaffected: single-origin keeps working
(NB_WEB_URL falls through to NB_API_URL); split-origin operators set
NB_WEB_URL explicitly.
Plain 'bun run dev' (no NB_WEB_PORT in env) was leaving NB_WEB_URL unset, so the OAuth callback redirected to NB_API_URL (the API port) and the user landed on a workspace_error JSON page instead of the Connections tab. The web's vite.config.ts already defaults to port 27246 when NB_WEB_PORT isn't explicit. Mirror that default in the dev supervisor so 'bun run dev' Just Works without extra env. Skipped when --no-web (no SPA running, so a callback redirect target makes no sense).
The OAuth round-trip now redirects the user back to /settings/workspace/connections
after auth, but the code-exchange + tools/list runs asynchronously in
the background — the user lands while the bundle is still
'pending_auth'. Without a refresh signal, the card stuck at
'Connecting…' until the user manually reloaded.
Subscribe ConnectionsTab to the connection.state_changed SSE event;
call refresh({silent:true}) on every transition for the active
workspace. Silent skips the loading flicker since the user already
has populated data on screen.
Token comes from getAuthToken() (module-level state, set on login).
useEvents handles the SSE connection lifecycle (open on mount, close
on unmount). Two SSE connections per browser — App's global one and
this page's — is fine; SSE is cheap and the page is rarely the
top-level focus.
…ons tool
Personal connections now follow the user across workspaces. Workspace
connections stay scoped to their workspace. The conceptual mismatch the
two-column-page-in-workspace-settings was hiding is gone:
- workspace-scope tokens → `workspaces/<wsId>/credentials/...`
(shared by every workspace member, lifecycle of the workspace)
- user-scope tokens → `users/<userId>/credentials/...`
(personal, available across every workspace the user is in,
survives leaving any workspace)
# Architecture
WorkspaceOAuthProvider takes `owner: OAuthOwnerContext` — a discriminated
union over { workspace, wsId } | { user, userId } — instead of the old
`wsId + memberId?` pair. Storage paths derive from the owner; the
member sub-folder is gone (each owner gets a single per-(owner, server)
directory). DCR client.json is per-owner: workspace-scope still shares
one client across members; user-scope is per-user (correct OAuth
semantics for personal — my Granola is mine).
`UserConnectionStore` (parallel to WorkspaceStore) manages
`users/<userId>/user.json` listing personal bundles. Lifecycle gains
`userInstances` keyed by (server, user) and a `workspacesForUser`
resolver so when a personal install lands or boot replays user.jsons,
each user's bundles get registered as a UserPoolSource entry in every
workspace they're a member of. Pool dispatches per-call by principalId
to the user's source.
# Vocabulary rename
`oauthScope: "member"` is gone everywhere. New vocabulary:
`oauthScope: "user"`. Same for catalog `defaultScope`,
`MemberPoolSource` → `UserPoolSource`, `memberPools` → `userPools`,
`memberId` → `userId`. The old vocabulary was a workspace-context
shortcut for "per-user-within-workspace" — a half-measure that user-
global storage replaces.
# Tool replaces routes
REST routes `/v1/connections/{catalog,installed,install,disconnect}`
are gone. The web shell calls `manage_connections` via /v1/tools/call
(catalog → list_catalog, installed → list_installed, install, disconnect)
— matches the convention that platform features are MCP tools
(reachable to Claude Code, Cursor, etc., not just the web UI).
`/v1/mcp-auth/{initiate,callback}` stay routes — initiate sets a
session-bound state cookie, callback IS a browser redirect target.
# Frontend
ConnectionsTab.tsx → ConnectionsPage.tsx at top-level `/connections`
(sibling of Conversations, Files). Two stacked sections:
- "Personal — Your account" (user-scope catalog entries)
- "Workspace — Shared with your team" (workspace-scope entries)
Sidebar gets a static "Connections" entry below dynamic placements
(global concern, doesn't fit per-workspace placement model). Removed
from `/settings/workspace/connections`.
InstalledConnection wire shape flattened: { scope, state, identity?,
... } instead of the old myConnection/workspaceConnection split.
# Verification
- `bun run check` clean (server)
- `bun run check:web` clean
- `bun run verify:static` clean (1 pre-existing url-validator warning)
- `bun run test:unit` — 2441 pass, 0 fail
- `bun run test:integration` — 460 pass, 6 skip, 0 fail
- `bun run smoke` — 17 pass, 0 fail
Phase 0 of the connectors restructure: rename the package/integration
concept from "connection" to "connector" throughout the codebase. The
word "connection" is preserved only where it refers to the live OAuth/
auth state of an authenticated bundle (Connection state machine, the
connections map on BundleInstance).
Renames:
- manage_connections tool → manage_connectors
- src/tools/connection-tools.ts → src/tools/connector-tools.ts
- UserConnectionStore → UserConnectorStore
- src/users/user-connection-store.ts → src/users/user-connector-store.ts
- src/connections/ → src/connectors/
- ConnectionCatalogEntry → ConnectorCatalogEntry
- DEFAULT_CONNECTION_CATALOG → DEFAULT_CONNECTOR_CATALOG
- WorkspaceStore.connectionsAllowList → connectorsAllowList
- Web wrappers: getConnectionsCatalog → getConnectorsCatalog,
getInstalledConnections → getInstalledConnectors,
installConnection → installConnector, disconnectConnection →
disconnectConnector
Sets up subsequent phases: settings restructure (kill top-level
/connections, two scoped tabs at /settings/{personal,workspace}/
connectors), Configure detail page with tool permissions, and
permission enforcement at tools/call dispatch.
… table The Configure permissions table was showing the full prefixed tool name (`notion-org__notion-search`) and the entire MCP-flavored description — multi-paragraph instructions, embedded `<example>` JSON blobs, hundreds of words per tool. Wall-of-text. Two fixes: - Backend: `manage_connectors.list_tools` strips the `<serverName>__` prefix before returning. The Configure page is single-connector context; the prefix is internal dispatch plumbing. Storage layer already keys on bare names, so this just makes the API contract consistent. - Frontend: `summarizeToolDescription` collapses the description to the first sentence (or first line, max 140 chars), strips XML-ish `<example>` tags, and parks the full text in the row's `title` tooltip. Row height stays predictable; full description still reachable on hover.
A connector is a connector. The Connectors page is now the canonical home for everything installed in a workspace — local stdio bundles, local URL bundles, Synapse apps, remote OAuth services. Browse for new ones happens on a separate /browse page. - Backend: `manage_connectors.list_installed` now walks every bundle visible in the workspace registry (replaces the old catalog-only filter). Adds `bundleName`, `version`, `type: "remote" | "local"`, `toolCount`, `trustScore`. Catalog enrichment stays optional. - Backend: new `manage_connectors.uninstall` action. For OAuth bundles: revoke tokens upstream, then `lifecycle.uninstall` (remove from workspace.json, stop source, clear credentials, drop tool permissions). For local bundles: just `lifecycle.uninstall`. - UI: ConnectorList is now a flat list — no cards. Each row: icon (catalog iconUrl or hashed-initial fallback), name, optional status text only when something needs attention (reauth / failed / not connected). Whole row is a link to Configure. - UI: search input appears when there are >5 installed bundles. - UI: Browse button on each tab → new ConnectorBrowsePage. Stub for v1 — surfaces the catalog filtered by scope, with a placeholder note pointing at future sources (mpak.dev, per-org directories, custom URLs). - UI: Configure page wired to the new uninstall flow (full removal, not just OAuth revoke). Drops "Reconnect" button for local bundles (OAuth-flow specific). - About tab: bundle table removed — Connectors is the canonical surface. About now just shows platform version + a link.
Browse was happily firing Install on already-installed catalog entries,
which then 500'd at the lifecycle layer ("principal '_workspace'
already connected"). Cosmetic gap, real friction.
- Browse fetches installed connectors alongside the catalog and
matches by URL (the stable identity).
- Two sections: not-yet-installed (primary, top) and "Already
installed" (demoted, with smaller heading).
- Already-installed rows render a subdued "Installed · Configure"
link straight to the Configure detail page — no more disabled
Install button that produces a backend error on click.
- Icon dimmed to 60% on already-installed rows so the eye finds new
things up top.
Browse no longer queries a single hardcoded catalog. It now asks the DirectoryAggregator for entries from every enabled registry, mapped into a uniform DirectoryEntry shape. Curated remote services and mpak bundles render side-by-side with the same row template; the install action dispatches by entry type. - src/registries/types.ts: ConnectorRegistry + DirectoryEntry + RegistryConfig + InstallAction discriminated union (remote-oauth / mpak-bundle / direct-url). - src/registries/registry-store.ts: file-backed instance-level config at <workDir>/registries.json. Auto-seeds with curated (locked) + mpak.dev (default-on) on first read. Atomic writes. - src/registries/curated-registry.ts: wraps the in-process catalog as a ConnectorRegistry returning remote-oauth entries. - src/registries/mpak-registry.ts: stub returning a small hand-curated set of well-known mpak bundles. TODO marks the wire-up point for real mpak.dev API integration. - src/registries/aggregator.ts: builds active registries from config, concatenates entries, isolates per-registry failures into the result so one bad source doesn't blank the page. - manage_connectors.list_directory action consumes the aggregator; the existing list_catalog stays for backwards compat. - New manage_registries tool: list (any caller), enable / disable / set_url / rename (org admin or owner only). - Browse page now consumes list_directory; renders a small "Curated" / "mpak.dev" badge per row; mpak-bundle entries show "Install via mpak — coming soon" with a deep link to mpak.dev. - New Settings → Organization → Registries admin page with per-registry enabled toggle and editable URL field for mpak. Single-org per instance assumption is locked in: registries.json lives at the work-dir root, no per-org subdirectory.
…stall self-heals Two related bugs causing "Bundle X not installed" after uninstall→install: 1. lifecycle.uninstall clears its `instances` map and removes from the legacy global nimblebrain.json, but never touches workspace.json#bundles[]. That array was added later for catalog- installed connectors and the lifecycle wasn't updated to clean it. Result: after uninstall, workspace.json has a stale entry; the next install sees the dup, reports alreadyInstalled, skips seedInstance, and the OAuth initiate fails with a 404 from lifecycle.getInstance returning null. Fix: handleUninstall now removes the matching entry from workspace.json#bundles after lifecycle.uninstall. Same fix for user-scope: removes from users/<id>/user.json after disconnectUser. 2. Pre-existing orphan state (anyone who hit this bug before today's fix shipped, plus any future state-divergence) needs to recover on the next install attempt. handleInstall now self-heals: when the dup check matches but lifecycle has no instance for that serverName, re-seed instead of returning alreadyInstalled. Same pattern in both workspace and user paths.
The callback was hardcoded to redirect to /settings/workspace/connections
— a path that no longer exists since the rename to /connectors and the
split into Personal vs Workspace tabs. Result: blank page after
authorize.
Two changes:
- resolveWithCode now returns the matched flow's metadata
(`{ wsId, serverName }`) instead of a bare boolean. Callers can
decide what to do with the now-resolved flow.
- mcp-auth callback uses the metadata to look up the bundle's
oauthScope on the lifecycle, then redirects to either
/settings/personal/connectors (user-scope) or
/settings/workspace/connectors (workspace-scope). Falls back to
workspace if the lookup fails.
Tests updated for the new return shape.
… padding
Two unrelated polish items shipped together because they share files.
1. Personal Connectors UI removed.
The Personal Connectors page was empty for almost every user — none of
the seeded catalog entries land in user scope today (mpak entries
default to workspace; the user-scope catalog services like Granola,
Gmail, etc. require operator setup that v1 doesn't surface). Showing
an empty page tagged "Personal" was confusing without explaining why.
What changed:
- "Personal" group dropped from Settings nav (PLATFORM_SECTIONS,
SectionGroup type, GROUP_LABELS, render loop)
- PersonalConnectorsTab.tsx deleted
- /settings/personal/connectors{,/browse,/:serverName} routes now
redirect to /settings/workspace/connectors so existing links don't
404
- /connections (legacy) redirect target updated to workspace
- OAuth callback always lands on /settings/workspace/connectors;
user-scope dispatch reserved for when Personal returns
What stayed: the user-scope backend pathways (UserConnectorStore,
lifecycle.seedUserInstance / disconnectUser, OAuth provider's
owner-context discrimination, manage_connectors `scope: "user"`
parameter). The abstraction is intact — only the UI surface is parked.
2. Browse/Configure padding aligned with the rest of Settings.
The parent SettingsPage already wraps content in `p-4 md:p-6`. The
new Browse/Configure pages were adding their own `px-4 py-6 md:px-6
md:py-8` on top, doubling the gutter and pushing the content
inward. Other settings pages (UsersTab, OrgRegistriesTab,
WorkspaceConnectorsTab) just use `max-w-{3xl,5xl} mx-auto space-y-6`
and inherit the page padding from the parent. Both pages now match.
…ions
Static-auth catalog entries (Asana, HubSpot, Gmail, Outlook, Zoom)
needed an in-product setup path. CLI-only seeding was a real friction
gap: Browse showed "Operator setup required" with a portal link but
no way to actually configure the OAuth app without dropping to a shell.
Architecture:
- Public client_id lives on workspace.json#oauthOperatorApps[catalogId]
with an audit trail (configuredAt, configuredBy). It's not a secret;
parking it in workspace config is honest about its nature and keeps
the credential store free of non-secret values.
- Private client_secret stays in the workspace credential store at the
catalog's clientSecretKey (renamed from credentialKey for clarity).
- Lifecycle: operator setup is a *prerequisite* for install, not a
peer of it. Bundle install/uninstall does not touch operator
config. Explicit removal goes through remove_operator_setup, which
refuses to run while the connector is installed.
New manage_connectors actions:
- setup_operator { catalogId, clientId, clientSecret } — upsert. ws
admin gated. Persists secret first, then stamps clientId + audit
trail. Cleans up the orphan secret if the workspace.json write
fails. Calling this on already-configured entries rotates both.
- remove_operator_setup { catalogId } — admin gated. Refuses while
connector is installed (uninstall first; protects the BundleRef's
credential pointer from going stale mid-OAuth).
Install path:
- Static-auth handleInstall now resolves the operator config:
workspace.oauthOperatorApps[id].clientId + credential at
clientSecretKey. Plumbs both into BundleRef.oauthClient. Refuses
early with a clear next-step message if either is missing.
Surface:
- DirectoryEntry.operatorConfigured: boolean — true for static-auth
entries that have both pieces in place. CuratedRegistry computes
it via a per-call ListEntriesContext.isOperatorConfigured closure
(workspace-aware seam without coupling registries to the runtime
singleton). DCR / mpak / direct-url leave it undefined.
UI:
- New OperatorSetupModal: portal link + hint + two fields. Same
component serves first-setup and rotate; clientId pre-fillable,
secret never echoes. Esc / outside-click closes; auto-focus first
empty field.
- Browse row affordances:
- not configured + admin → "Set up" button
- not configured + non-admin → "Operator setup required" pill (existing)
- configured + admin → "Install" + small "Edit OAuth app" link
- configured + non-admin → "Install"
- already installed → standard "Configure" path
Catalog: operatorSetup.credentialKey → clientSecretKey (clearer);
workspace store update() patch list extends with oauthOperatorApps.
Three new test files (31 tests, all passing):
- test/unit/registry-store.test.ts (7): seeds defaults on first read,
persists across instances, locked registry refuses disable but
allows rename, unknown id throws, auto-restores hand-edited curated.
- test/unit/directory-aggregator.test.ts (7): aggregates enabled
registries, disabled ones don't surface, attribution intact, dedupes
on (registryId, id), passes ListEntriesContext through to registries
for workspace-aware fields, DCR entries leave operatorConfigured
undefined, works without context.
- test/unit/connector-tools.test.ts (17): setup_operator (admin gate,
upsert semantics, persists both pieces, error paths), remove_operator_
setup (admin gate, refuses while installed, removes both), list_
directory (curated + mpak entries, operatorConfigured reflects
state), install (static-auth refused without setup, succeeds with
proper BundleRef.oauthClient when configured).
Plus a defensive fix on isWorkspaceAdmin: tolerate a workspace record
missing the members array (corrupted file, partial migration). Org
admins still pass; otherwise we deny rather than throw — fail-closed
posture for an authorization helper.
Plus an integration-test stale-assertion fix: workspace-oauth-provider
test was still expecting resolveWithCode to return boolean. Updated to
match the now-shipped `{wsId, serverName} | null` return shape.
mgoldsborough
changed the title
…lback returnUrl Defense-in-depth: three places where a string-shaped URL flowed into either an `<a href>` / meta-refresh target or a hostname-display call without a protocol allowlist. None are user-controlled (catalog ConfigMap, org admin, operator env), but the existing iconUrl gate sets the precedent — be symmetric. - catalog: extend isSafeIconUrl validation to operatorSetup.portalUrl in load-catalog.ts. Drops the entry with a logged warning instead of letting `javascript:` / `data:` schemes reach the Setup modal. - web: new lib/safe-url.ts with safeHostname(). Replaces the raw `new URL(...).hostname` calls in OperatorSetupModal + ConnectorBrowsePage that would otherwise throw + crash the surface if a malformed URL slipped past server-side validation. - manage_registries.set_url: enforce parsed.protocol === "http:" || "https:" so org admins can't paste `javascript:` / `file:` URLs into the mpak registry config field. - mcp-auth callback: validate the assembled returnUrl's protocol; fall back to a same-origin relative path if NB_WEB_URL / NB_API_URL resolves to something non-http(s). escapeHtml only escapes `&<>"'` — a `javascript:` scheme survives without this guard. No tests changed; existing suite stays green (2480 unit). The catalog-load regex dropping malformed portalUrl entries is exercised implicitly by the static-auth catalog sanity test.
Address QA findings W5 + S1, S3, S5, S7, S8 in one cleanup pass.
- Delete unused `hasPersistedMemberOAuthTokens` from oauth-tokens.ts.
Was a leftover from the pre-rename `members/<memberId>/...` storage
layout; user-scope tokens moved to `users/<userId>/...` and never
needed a parallel boot probe.
- Delete unused `getPendingAuthUrl` from BundleLifecycleManager (and
its tests). The route that consumed it was removed; tests were the
only callers.
- PermissionStore.save uses `dirname(path)` from node:path instead of
`path.substring(0, path.lastIndexOf("/"))` — cross-platform safety
+ matches the rest of the codebase.
- MpakRegistry canonicalizes the configured base URL via
`new URL(...).origin` instead of regex-stripping a trailing slash.
Rejects malformed values that slipped past `set_url` validation,
hands every callsite one shape to compose against.
- Sweep stale "member" / "members" vocab in BundleInstance type
comments + the surrounding oauthScope JSDoc — the type literal
renamed to "workspace" | "user" but the doc lagged.
- Sweep stale `/v1/connections/installed` references in workspace-
runtime + lifecycle comments — those routes are gone; replaced
with `manage_connectors.list_installed` references.
No behavior change. Test count drops by 2 (the deleted
`getPendingAuthUrl` cases); rest of the suite is green.
…de shape
Closes QA findings W4 + S2 + S4.
- New test/unit/registry-permission-enforcement.test.ts (5): proves the
ToolRegistry.execute permission gate wires correctly to the
PermissionStore. Covers default-allow when no context configured,
default-allow when context is configured but no policies set,
disallow short-circuit with structured tool_permission_denied error
before source.execute fires, explicit allow round-trip, and
per-connector scoping (disallow on connector A doesn't block
same-tool-name on connector B). PermissionStore had its own coverage
but the dispatch wiring did not.
- ConnectorBrowsePage dedupes the mount fetcher and post-modal-save
refresh into a single useCallback-memoized fetchDirectory. Previously
both useEffect mount and a separate `refresh()` helper made identical
Promise.all([listDirectory, getInstalledConnectors]) calls. First
render is one fetch now instead of two. Drops the dead-intent
`operatorApps` state machinery alongside (was always populated with
empty clientId strings, no functional difference from no pre-fill).
- Revert resolveWithCode to boolean. The metadata-shape return
(`{wsId, serverName} | null`) was added in anticipation of scope-
aware OAuth-callback redirects when Personal Connectors UI returns;
Personal got parked, the metadata was discarded with a `void matched`
smell. Cleaner to ship the actual current state — boolean — and let
scope-aware dispatch reintroduce the metadata when there's an actual
consumer. Tests + integration assertions reverted to match.
QA W1. JSON.stringify produces a valid JS string literal but does NOT escape `</script>` / `<!--` — sequences that close script context even though they survive both the protocol-allowlist guard and escapeHtml. The threat is bounded (NB_WEB_URL is operator-controlled, with a host-header-poisoning fallback through c.req.url origin), but defense-in-depth is the right posture: the inline-script body is the only spot where the returnUrl reaches script context, and it was unprotected. Fix is a single-line: encode `<` as `<` after JSON.stringify. Surrounding href attribute and meta-refresh keep using safeReturnUrl (escapeHtml) — those sit in HTML attribute context, where the existing escape covers the relevant metacharacters.
…, S4, S5) QA pass closeout — four small, principled changes. W2: handleListDirectory's isOperatorConfigured closure was issuing workspace.json + credential reads once per static-auth catalog entry. With the 5 current entries that's 10 sequential disk reads on every Browse load, and it scales linearly with the catalog. Hoist the workspace fetch + credential-store handle outside the closure — single workspace read shared across all probes. S3: user-pool-source.ts had its own POOL_WORKSPACE_PRINCIPAL = "_workspace" literal alongside bundles/connection.ts's WORKSPACE_PRINCIPAL_ID. Both the same string, drift waiting to happen. Re-export from the canonical source. Existing callers keep working unchanged (POOL_WORKSPACE_PRINCIPAL is still exported, just sourced from the shared constant). S4: handleSetPermissions silently accepted writes for non-existent serverNames — the entries would sit unused since the runtime gate keys on installed-source dispatch. Reject up front via lifecycle.getInstance (or getUserInstance for user scope). Surfaces typos at write time. New test covers it. S5: pre-existing biome useOptionalChain warning in url-validator.ts. Was suppressed throughout this branch as a "Found 1 warning" line in lint output. Apply the auto-fix; lint output is now fully clean.
QA W1 + W3. W1 — ToolRegistry.execute previously fell through to source.execute when isUserScoped && !principalId (owner = null → policy check skipped). UserPoolSource self-protects further down the call path, but "couldn't identify the caller, so skip the policy gate" is the wrong fail-default for a security check. Refuse with a structured `principal_required` error before dispatch. New unit test covers the branch. UserPoolSource starts empty so the gate short-circuits before ever reaching execute() — proves the short-circuit semantically, not just structurally. W3 — `/v1/mcp-auth/initiate` was echoing raw `err.message` into the 500 response body. Workspace-authed callers, so the threat is narrow, but the body crosses trust boundaries (proxies, dev-tools, HAR exports). Log raw server-side via `cli/log`; return a generic "Failed to start OAuth flow. Check server logs for details." with the same `auth_start_failed` error code. No callers asserted on the previous error text.
Four stores (RegistryStore, PermissionStore, UserConnectorStore, WorkspaceStore) each shipped a near-identical `uniqueTmpSuffix()` + write-rename copy. Hits the duplication threshold; one shared helper in src/util/atomic-json.ts replaces all four. Tightening alongside: the helper writes mode 0o600 unconditionally. RegistryStore + PermissionStore previously wrote without an explicit mode (process umask default). Both files sit alongside credentials in the work-dir, and a single platform UID owns them at runtime — no functional change, just structurally-enforced safer posture. Five tests in test/unit/atomic-json.test.ts: pretty-print + trailing newline, mode 0o600, repeated overwrite without tmp leakage, concurrent-write tmp cleanup, write-failure path doesn't leak tmp. Final: 2490 unit + 466 integration, lint clean.
This was referenced May 7, 2026
Earlier in this PR we removed the About tab's bundle table on the "Connectors is canonical" architectural argument. Correct in steady state — but the Connectors page is brand-new code, the PR is large (~9k lines, 21 commits), and About was a known-good fallback that didn't depend on any of the new paths (manage_connectors tool, registry abstraction, permission store). Restoring the table reduces the blast radius of this PR: if anything in Connectors regresses on first contact with production data, operators have a stable read-only surface that uses the unchanged list_apps path. Added a small Connectors pointer at the top of the section so the two surfaces don't feel like a hidden duplicate — About stays read- only; Connectors is where you actually manage things. When Connectors is proven (a few weeks in production, edge cases shaken out), we'll re-remove the table in a follow-up commit. Tracked as a separate issue.
This was referenced May 7, 2026
mgoldsborough
added a commit
that referenced
this pull request
May 8, 2026
Three follow-ups from the second QA pass on PR #166: - Permission gate now treats `WORKSPACE_PRINCIPAL_ID` ("_workspace") the same as a missing principal at the registry layer. The OAuth initiate path is the only caller that mints the sentinel and it doesn't reach `registry.execute` today, but the constant is exported and a future caller leaking it would otherwise key user-scope policy lookups on `users/_workspace/`. UserPoolSource still refuses the call downstream — this just keeps the gate authoritative. - `handleSetupOperator` writes to the credential store before the workspace.json update. If the metadata write fails, the previous shape orphaned the secret. Wrap the second write in try/catch and best-effort delete the credential we just put — but only when no prior secret existed. On a rotate, leaving the prior valid secret in place is better UX than wiping a working credential because the metadata write hiccupped. - Correct the misleading "fsync via the rename" comment in atomic-json. Rename is atomic for concurrent readers (consistency) but does not fsync the data (durability). Spell out the tradeoff so the next reader doesn't think the durability claim was made. Includes two regression tests on the orphan-rollback path: the no-prior-secret case (delete fires) and the rotation case (delete is suppressed).
mgoldsborough
added a commit
that referenced
this pull request
May 8, 2026
* test: nb credential CLI happy-path coverage + oauth-tokens probe (#184) Closes #184. Six integration tests in test/integration/cli-credential.test.ts covering the four `nb credential` subcommands operators rely on for seeding OAuth client_secret references: - set: writes to <workDir>/workspaces/<wsId>/credentials/secrets/<key> with mode 0o600 - get: round-trips the value via stdout, no trailing newline (piping- friendly), exits 1 with stderr diagnostic when the key is missing - list: shows seeded keys but never the values - delete: removes the file, idempotent on a missing key - full lifecycle: set → delete → get exits non-zero Each test uses --work-dir against a per-test mkdtempSync directory so the suite is hermetic. Plus three unit tests in test/unit/oauth-tokens.test.ts covering hasPersistedWorkspaceOAuthTokens — a small file-existence probe used at platform boot to pick the right initial Connection state for URL bundles. Trivial but uncovered before now. * test: install → set policy → enforce end-to-end (#183) Closes #183. Six integration tests in test/integration/policy-enforcement-end-to- end.test.ts proving the full pipeline: write a per-tool policy via the manage_connectors tool surface, then watch ToolRegistry.execute short-circuit the corresponding call with a structured tool_permission_denied. PermissionStore + the registry's gate + manage_connectors all participate; previously each had unit coverage in isolation but the boundary was uncovered. Cases: - disallow round-trip: policy → call → tool_permission_denied; source.execute never invoked - allow round-trip: explicit allow lets the same tool through - default-allow: a tool without a recorded policy passes through even when its sibling is disallowed - rotate: flipping a tool from disallow to allow lets a previously- blocked call through - get_permissions reflects the just-written state - restart hydration: a fresh PermissionStore on the same workDir sees persisted policies and the gate fires identically Stubs Runtime to the ~5 methods the handlers need (workspace store, work dir, registry store, permission store, lifecycle, registry). Real WorkspaceStore + RegistryStore + PermissionStore + ToolRegistry + BundleLifecycleManager instances drive the test — pure stub layer is just the runtime boundary. * fix(oauth): thread AbortSignal through redirect-probe fetches (#182) Lifecycle's 15s startAuth timeout previously raced against the provider's redirect-probe loop without any way to actually cancel an in-flight fetch. An unresponsive auth server would hold the TCP read open for the platform's full network deadline (often 30–60s), outliving the timeout and burning sockets in the background. WorkspaceOAuthProvider now accepts an optional abortSignal threaded into every outbound fetch (redirect probes and revocation requests). BundleLifecycleManager.startAuth creates an AbortController, hands its signal to the provider, and aborts in the finally block of the Promise.race — cancelling probes promptly on both timeout and success paths. Includes a regression test that drives a hanging mock auth server, fires the abort 100ms in, and asserts the fetch rejects within 1500ms (well under the platform fetch default). * fix(oauth): extend AbortSignal threading to revocation path The previous commit (#182) wired the controller into the redirect-probe fetch only. The provider's docstring and the PR body both claimed the signal also covered `revokeAndDeleteTokens`, but the AS-metadata discovery and the RFC 7009 POSTs in `postRevoke` still ran on a bare `fetch` and would outlive an unresponsive server's full network deadline. Wrap the fetcher inside `revokeAndDeleteTokens` so every call site (discoverAuthorizationServerOrigins, the AS-metadata GET, and the revocation POSTs) receives `this.abortSignal`. No call site in this path sets its own `init.signal`, so the spread is unambiguous. * fix(connectors): QA round 2 — sentinel guard, rollback, doc fix Three follow-ups from the second QA pass on PR #166: - Permission gate now treats `WORKSPACE_PRINCIPAL_ID` ("_workspace") the same as a missing principal at the registry layer. The OAuth initiate path is the only caller that mints the sentinel and it doesn't reach `registry.execute` today, but the constant is exported and a future caller leaking it would otherwise key user-scope policy lookups on `users/_workspace/`. UserPoolSource still refuses the call downstream — this just keeps the gate authoritative. - `handleSetupOperator` writes to the credential store before the workspace.json update. If the metadata write fails, the previous shape orphaned the secret. Wrap the second write in try/catch and best-effort delete the credential we just put — but only when no prior secret existed. On a rotate, leaving the prior valid secret in place is better UX than wiping a working credential because the metadata write hiccupped. - Correct the misleading "fsync via the rename" comment in atomic-json. Rename is atomic for concurrent readers (consistency) but does not fsync the data (durability). Spell out the tradeoff so the next reader doesn't think the durability claim was made. Includes two regression tests on the orphan-rollback path: the no-prior-secret case (delete fires) and the rotation case (delete is suppressed). --------- Co-authored-by: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com>
Ovaculos
added a commit
to Ovaculos/nimblebrain
that referenced
this pull request
May 11, 2026
… content type Two leftovers from rebasing onto upstream/main after the Connectors UX landed in NimbleBrainInc#166/NimbleBrainInc#186/NimbleBrainInc#195 and the canonical-id slug refactor moved the helpers around: 1. `bundleEntryMatchesTarget` had already been extracted upstream (system-tools.ts:76); my Fix 3 commit redeclared it lower in the same file, which biome flagged as `lint/suspicious/noRedeclare`. Removed the duplicate and kept the upstream definition — the call sites already reference the right binding by name. 2. `AboutTab.tsx`'s upload error-extraction filter narrowed to `{ type: string }` only, so the subsequent map produced `text: string | undefined` (web's content schema marks it optional) and failed `cd web && bunx tsc --noEmit`. Tightened the filter to a type predicate that asserts both `type === "text"` and `typeof text === "string"`, so the map call sees a strict `{ type: "text"; text: string }`. Verified locally: bun run verify:static, test:unit (2593/0), test:web (257/0), test:integration (480/0/12 skip). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR has grown well beyond its original Tracks A+B+C scope. The
final shape is a coherent unification of every install-and-manage
surface in NimbleBrain into a single primitive — the connector —
backed by a registry abstraction, per-tool permissions, and an in-
product operator-setup flow.
The conceptual model
A connector is anything that exposes tools or UI surfaces to the
agent: local stdio bundles, local URL bundles, Synapse apps, remote
OAuth services. They flow through one list, one Configure surface,
one install/uninstall path. Differences (transport, auth, UI) are
implementation details surfaced as badges, not separate primitives.
Architecture
Registries
src/registries/— pluggable sources of installable connectors.RegistryStore— instance-level config at<workDir>/registries.json.Auto-seeds on first read with
curated(locked) andmpak(defaulton, URL-overridable). Atomic writes.
ConnectorRegistryinterface +DirectoryEntry(uniform shape) +InstallActiondiscriminated union (remote-oauth|mpak-bundle|
direct-url).CuratedRegistrywraps the in-process catalog. ComputesoperatorConfiguredper-call via aListEntriesContextresolver —workspace-aware seam without coupling registries to the runtime
singleton.
MpakRegistryis a stub returning a small hand-curated set ofwell-known mpak bundles. Real mpak.dev API client + bundle install
are separate-PR follow-up.
DirectoryAggregatorbuilds the active registry list, isolatesper-registry failures into
errors[]so one bad source doesn'tblank Browse.
Permissions
src/permissions/— per-tool policy enforcement.PermissionStore— file-backed atusers/<userId>/permissions.jsonandworkspaces/<wsId>/permissions.json. Default-allow when no entryexists. Atomic writes.
ToolRegistry.execute()consults the store before dispatching.UserPoolSourcecalls key on the calling principal; everythingelse keys on workspace. Disallowed tools return a structured
tool_permission_deniederror.Operator OAuth app setup
workspace.json#oauthOperatorApps[catalogId]carries the publicclient_idplus an audit trail (configuredAt, configuredBy). Thematching
client_secretlives in the workspace credential store atoperatorSetup.clientSecretKey. Independent of bundle lifecycle —uninstall doesn't touch it. New
manage_connectorsactions:setup_operator(admin-gated upsert) +remove_operator_setup(refuses while installed).
UX
Settings → Workspace → Connectors (
/settings/workspace/connectors)Flat list. One row per installed bundle: icon, name, status nudge
only when something needs attention (reauth / failed). Whole row
links to Configure. Search appears at >5 items. Top-right
Browsebutton.
Browse (
.../connectors/browse)Aggregated directory across all enabled registries. Curated +
mpak.dev entries render side-by-side with the same row template;
install dispatch follows the entry's
installdiscriminator.Per-row
Curated/mpak.devbadge for attribution. Already-installed entries demote to bottom of the list. Static-auth
entries:
Set upbutton → modalOperator setup requiredpillInstall+Edit OAuth applinkInstallOperatorSetupModal
Same component for first-setup and rotate. Portal link, two fields,
clientId pre-fillable (secret never echoes). Esc/click-outside closes.
Configure (
.../connectors/:serverName)Centered, no card chrome. Top-right action bar: optional Docs link
Uninstall(text-style destructive). Header: icon + name +metadata (type, scope, state, version, connected-as).
Reconnectinline when reauth needed.
ToolPermissionsTablebelow: per-toolAllow / Disallow toggles, optimistic with rollback, bulk
Allow all/Disallow all. MCP descriptions trimmed to first sentencewith full text in row's
titletooltip.Settings → Organization → Registries
Admin-only. Each registry has an enable/disable toggle (curated is
locked-on); mpak has an editable URL field for private mpak
instances.
Notable bug fixes along the way
lifecycle.uninstallcleared its owninstancesmap and the legacy globalnimblebrain.jsonbutdidn't touch
workspace.json#bundles[]. Reinstall after uninstallfailed silently then 404'd at OAuth initiate. Fixed at the connector-
tools layer + self-heal in
handleInstall./settings/workspace/connections(renamed to
/connectors) caused a blank page after authorize.Callback now redirects to the workspace connectors tab.
Connection.sourcewhich is only populated on the user-flow OAuth path — boot-restored
bundles have
source: nulleven when running. Now reads from theworkspace registry, which is the authoritative location.
Personal Connectors UI parked
/settings/personal/connectorswas empty for nearly every user (nocatalog entries land in user scope today; the user-scope services
require operator setup the v1 doesn't surface). Removed from the nav
UserConnectorStore, lifecycle user-scope methods,
manage_connectors scope: \"user\") stay intact —the abstraction is unbroken; only the UI surface goes. Routes
redirect to
workspace.About tab
"Installed Bundles" table removed. Connectors page is now the
canonical surface; About just shows platform version + a link.
Test coverage
4 new unit-test files (38 tests):
permission-store.test.ts(8) — default-allow, set/get round-trip, scope isolation, path-traversal defense
registry-store.test.ts(7) — seed defaults, persist acrossinstances, locked semantics, auto-restore curated
directory-aggregator.test.ts(7) — aggregation, attribution,dedupe, context plumbing
connector-tools.test.ts(17) — setup_operator, remove_operator_setup, list_directory, static-auth install path
Total: 2480 unit + 466 integration + 17 smoke all passing.
bun run verifyclean.What's deferred
MpakRegistryis a stub.Surface is right; the API client +
kind: \"mpak-bundle\"installflow are net-new work. Best as a separate PR.
reason to surface it.
pieces have unit coverage; an end-to-end MCP-protocol test would
add value but isn't a prerequisite for ship.
Post-review additions (QA pass)
After QA review, three follow-up commits landed:
manage_registries.set_url, and validates the OAuth callback's returnUrl scheme. Defense-in-depth across operator-controlled URL surfaces.hasPersistedMemberOAuthTokens(orphaned by user-scope rename),getPendingAuthUrl(no callers post-route-removal), refines comments referencing/v1/connections/*and the oldmembervocab, swaps PermissionStore's substring slicing fordirname(), canonicalizes MpakRegistry URLs vianew URL(...).origin.registry-permission-enforcement.test.ts(5 tests) proves theToolRegistry.executepermission gate wires correctly. ConnectorBrowsePage dedupes mount + post-modal-save fetches into onefetchDirectory. RevertsresolveWithCodeto boolean since nothing consumes the metadata today; cleanly removes thevoid matchedsmell.Final test count: 2483 unit + 466 integration + 17 smoke.
bun run verifyclean.Filed as follow-up