[pull] main from activepieces:main

brain/knowledge/ai-intelligence/ai-agents.md

@@ -35,6 +35,14 @@ A flow step type (the `run_agent` action of `@activepieces/piece-ai`) that runs
   - **Credit is read from a status or the specific `insufficient_quota` marker, never loose patterns over a response body.** OpenAI signals billing exhaustion as a *retryable* 429 with the marker in the **body**, so credit is checked before the retryable verdict — but scanning a body for `credits`/`402` made a provider 500 whose HTML error page said "credits" complete as a billing failure and hide a real outage.
   - **`ENTITY_NOT_FOUND` counts only for an AI-provider `entityType`, and `VALIDATION` counts for nothing.** A bare not-found is our bug; the `VALIDATION` that reaches this surface is the conversation concurrency lock, and a conversation stuck `STREAMING` is a state worth keeping visible.
   A completed job stores no `errorMessage`, so the `warn` log carrying `agentRun.errorClass` is the only remaining record.
+- **A tool name is user text on four paths, and only the worker sees all of them.** `createToolName` is applied by the flow-tool dialog and the piece-tool stores, but a knowledge-base name was stored as typed and the AI piece's `toolName` is free `ShortText`. That string becomes the AI-SDK `ToolSet` key verbatim, which is how a name earned a 400 from Anthropic — *our* request, never the user's fault, which is why widening the status allow-list to 400 would have been the wrong fix. `mcpToolNameUtils.toValidToolName` is the guard, applied by `agentToolPolicy.withValidNames` in `execute-agent-run` — the one place the flow-step, chat and eval enqueue paths converge (`agent-conversation-controller` validates tool names not at all; `agent-run-controller` checks only the reserved prefix and duplicates, and does it on the *raw* names, so it cannot see a collision the rewrite creates). Four things it has to get right:
+  - **The pattern is the intersection of every provider we ship, not the one from the error we happened to see.** Anthropic's `^[a-zA-Z0-9_.-]{1,64}$` is the loosest: OpenAI and Bedrock reject `.`, and Gemini requires a leading letter or underscore. Guarding with Anthropic's rule leaves `handbook.pdf` — the obvious name for a knowledge base file — still failing everywhere else. The guard is `^[a-zA-Z_][a-zA-Z0-9_-]{0,63}$`.
+  - **It rewrites only a name that already fails**, because `createToolName` is not idempotent and re-running it would break the names `migrate-v16` persisted. `toValidToolName` re-checks its own output and re-derives from a prefixed source when `createToolName` returns a leading digit.
+  - **It dedupes, because sanitising converges.** `Company Docs` and `Company docs` map to one key, and every name with no `[a-z0-9_-]` at all used to hash identically — two CJK-named tools became the same key. `Object.fromEntries` is last-wins, so one tool vanished from the toolset with no error and answered from the wrong source. `createToolName` now hashes the original when the sanitised form is empty, and `withValidNames` is a list→list function holding a `taken` set.
+  - **MCP tools are left alone**, because `agent-mcp-client` already derives a sanitised key from `${toolName}_${name}`.
+  Server-side `toolName` is log-only — `executePieceTool` / `executeFlowTool` / `executeKnowledgeBaseTool` route on `piece`, `flowId` and `knowledgeBaseFileId` — so rewriting it breaks no lookup. `stepResultFrom` must be passed the knowledge-base tools too, or its `ToolCallType.KNOWLEDGE_BASE` branch is unreachable and the card shows the rewritten key instead of the file name.
+- **A retired model is user config, and only a marker in the message says so.** A provider 400 stays `internal` by default; one whose *message* matches `MODEL_UNAVAILABLE_PATTERNS` is `user`. Two deliberate narrowings, both learned the hard way: the body is **never** scanned, because any 400 carrying an HTML error page that says "deprecated" in its footer would launder our own outage; and a retired model on the managed `activepieces` key is **ours**, since `resolveModelIdForProvider` substitutes `curatedModels[0]` for anything uncurated — a stale constant in our repo failing every platform at once must page, not read as "the customer picked a bad model". That substitution still classifies as `user` on a BYO key, which is the residual gap.
+- **The curated chat lists rot silently and nothing checks them, but "the ticket said it's deprecated" is not evidence.** `ALLOWED_CHAT_MODELS_BY_PROVIDER` (`core/piece-types/src/lib/ai-providers.ts`) is the only thing deciding what the pickers offer; the models.dev catalog is metadata keyed by id and adds or removes nothing. Check a suspected-dead id against models.dev before deleting it — of the four ENG-466 named, only `grok-4.1-fast` had actually gone; the Gemini 2.5 pair was live, current and the *cheapest* Google option. Removing a live model is not a cleanup: `resolveModelIdForProvider` falls through to `curatedModels[0]`, so a BYO customer pinned to Flash would have silently moved to a Pro preview at roughly five times the token price, on their own key, with no notice and no migration. Three things move together when editing a list: `CHAT_MODEL_LABELS` (a curated id with no label fails `ai-providers.test.ts`), `MANAGED_MODEL_WEIGHTS` in `flow-run-ai-usage-tracker.ts` (its `?? 2` default is *below* the table's floor of 6, so a forgotten managed model under-bills — every `x-ai/*` id does today), and the **order**, since `curatedModels[0]` is both the picker's first row and the fallback for every unrecognised selection.
 - **Whatever enqueues an agent run must pre-check the same thing the worker resolves.** The chat route asked "is any provider enabled for chat" while the worker looked up the run's *pinned* provider, and the flow-step route checked nothing at all — so a run enqueued fine and could only fail. Both now call `agentHelpers.assertRunProviderConfigured`, which mirrors the worker's lookup. A pre-check that answers a *different* question than the worker is worse than none: it makes the failure look impossible.
 - **Everything the agent job does before its try/catch has no recovery.** `getAgentConfig` used to run outside it, so a config failure sent no error to the chat client and never called `releaseFlowStep` — the flow run sat PAUSED until `AP_PAUSED_FLOW_TIMEOUT_DAYS`. Anything added above that block needs its own failure path, or a paused run leaks.
 - **Build the unattended tool set as an allow-list.** Removing chat tools by name failed three times running — display tools, then build-plan and phase tools, then `ap_discover_action_auth` and `ap_load_guide`, which live with the local tools and so survived a filter written by tool group. Grouping tracks where a tool was constructed, not whether it assumes someone is reading. A flow step gets exactly what it is listed: its configured piece actions, the public-web readers, and the structured-output tool. Anything added to chat later stays out by default.
@@ -63,3 +71,6 @@ Paths verified 2026-07-17. An earlier version pointed at `packages/core/shared/s
 - **A knowledge base uploaded through the UI is not searchable.** Nothing in the upload path generates chunk embeddings; `knowledge-base.controller.ts` only *accepts* an embedding on a chunk. Chunks land with `embedding IS NULL`, and search filters those out, so the result is an empty answer rather than an error.
 - **`knowledge_base_chunk` is created by a migration that records itself as run even when pgvector is absent.** A database that gains pgvector later never gets the table, because the migration is already marked complete. Deleting its row from `migrations` replays it safely, since the DDL is `CREATE TABLE IF NOT EXISTS`.
 - **Embeddings are stored at a fixed 768 dimensions, and most models do not return that.** `text-embedding-3-small` answers 1536, and the `dimensions` provider option is namespaced under `openai`, so the OpenRouter and managed paths never see it. `agentAiUtils.toStorageEmbedding` truncates and re-normalises instead, which is what the option does server-side and works whatever the provider returns. This only holds for Matryoshka-trained models — adding a model that is not one will truncate badly and silently.
+
+- **Saving a saved agent publishes it.** `POST /v1/agents/:id` sets `goLive: true` unless the body says otherwise, so an ordinary save copies the draft over the published snapshot. There is no separate publish step in the UI, deliberately: two versions with no history means nobody can say which one a linked flow runs. The consequence is easy to trip over in tests and callers — anything that needs `draft` to differ from `published` has to write the row directly (`db.update('agent', id, { draft })`) or pass `goLive: false`, which is what the Test tab uses to stage a change it can run without shipping it. A test that edited the draft through the API to prove "a flow runs the published copy" was quietly moving the copy it was asserting about, and it only started failing when the save-publishes change merged from another branch.
+- **`ap_add_agent_tools` will save a tool with no connection pinned.** `connectionExternalId` is optional, so a tool the AI adds without one carries no `predefinedInput.auth` for good. The visible symptom is a connection picker card on *every* conversation with that agent, which reads as the card being broken or the credential expiring — it is neither. The agent has nothing to use, so it asks, and the answer only ever lands on the run (`__store_selected_connection` writes a map the agent tool set does not read), so the next conversation asks again. Fixing the card is the wrong end: pin a connection when the tool is created, and write a chosen or repaired one back into `draft.tools` via `editDraftTools`.

brain/knowledge/ai-intelligence/mcp-server.md

@@ -21,15 +21,35 @@ Exposes an Activepieces project as an MCP server so AI clients (Claude Desktop,
 
 ### How it works
 
-- Main protocol endpoint: `POST /v1/mcp/:projectId/http` (StreamableHTTP). Config: `GET/POST /v1/mcp/:projectId`, rotate token via `.../rotate`.
-- Auth: `Authorization: Bearer {token}` or `?token=`. OAuth 2.0 PKCE also supported for clients that need it.
+- Main protocol endpoint: `POST /mcp` at the domain root, plus `POST /mcp/platform` (StreamableHTTP), both registered in `server.ts`. Config lives under the project API (`GET/POST` on the project MCP server route).
+- Auth is **OAuth-only**: `resolveIdentity` accepts an `Authorization: Bearer` value only if `mcpOAuthTokenService.verifyAccessToken` verifies it as a signed JWT with audience `JwtAudience.MCP_OAUTH_ACCESS`. There is no static-token authenticator and no `?token=` query path.
 - AI pieces consume MCP tools over three transports: `SIMPLE_HTTP`, `STREAMABLE_HTTP`, `SSE`.
 - Embed SDK adds `authorizeMcp()` (in-embed OAuth consent), `mcpSettings()`, and `generateMcpToken()` (mints `{ mcpServerUrl, mcpToken }` with no OAuth flow, backed by `POST /v1/projects/:projectId/mcp-server/token` — a short-lived 15-min project-scoped token).
 
 ### Gotchas
 
+- **`mcp_server.token` is dead — nothing reads it.** It is written by the `getOrCreate` defaults and by both `/rotate` routes (`mcpServerService.rotateToken` / `rotatePlatformToken`), and consulted by **no authenticator**, so "rotating" it rotates a secret that grants nothing. It is still on the public `McpServer` zod schema, so the API keeps shipping a secret-shaped 72-char string that authenticates nothing — do not reach for it as a credential, and do not tell a self-hoster to. The settings panel is consistent with reality already (`mcp-credentials.tsx` renders the URL and *"Authentication is handled via OAuth"*, never a token). Deleting the column, the two routes, and the schema field is a breaking API-response change and has not been done.
+- **`mcp_oauth_token.clientKey` is decided once, at sign-in.** `exchangeCode` derives it from the registration's
+  redirect URIs via `mcpOAuthClientIdentity`, so the grants list can filter and group in SQL instead of loading
+  every `mcp_oauth_client` row on the platform to re-derive keys in memory. Two consequences: sharpening the
+  heuristic later does **not** relabel existing grants (they age out in 30 days, and an active client relabels on
+  its next refresh, which backfills a NULL key), and `NULL` is not a third state — it means "signed in before the
+  column existed" and reads as `unknown` everywhere, including the `?clientKeys=unknown` filter.
+- **Claude Code and Codex re-run Dynamic Client Registration on *every* sign-in**, registering the exact
+  ephemeral loopback port they are about to bind (`http://localhost:<port>/callback`,
+  `http://127.0.0.1:<port>/callback/<callback_id>`). So the exact-string `validateRedirectUri` works and
+  RFC 8252 port-agnostic matching is not needed — but a fresh `mcp_oauth_client` row and `clientId` is
+  minted per sign-in, so `clientId` is **not** a stable identity for "a connected client", and those rows
+  accumulate unbounded. Measured 2026-08-23 (Claude Code 2.1.235, Codex 0.149.0).
+- **Never advertise `client_id_metadata_document_supported`** in the authorization-server metadata while
+  `client_id` is validated against `^[A-Za-z0-9_-]{1,64}$`. Claude Code prefers a Client ID Metadata
+  Document, whose `client_id` is a URL; it only falls back to DCR because we stay silent about CIMD.
+  Advertising it without widening the `client_id` shape breaks Claude Code sign-in outright.
+- **A static `Authorization` header is worse than none for MCP clients.** In Codex, setting `bearer_token_env_var` or an `Authorization` header short-circuits to bearer auth and skips OAuth discovery entirely; in Claude Code a rejected `Authorization` header surfaces as a failed connection rather than falling back to OAuth. So a partially-built static-token path silently disables the OAuth path that does work. Related: headless/CI (`claude -p`, the SDK) has no `/mcp` panel and therefore no supported way to connect today.
+
 - Flow attribution: `ap_create_flow`/`ap_build_flow`/`ap_duplicate_flow` stamp `ownerId` (OAuth user) and `createdBy: { type: 'MCP', id }`.
 - `MCP_SERVER_CONNECTED` is deduped to at most one/user/server/day (`telemetryDedupe.onceToday`) — a daily-active signal, not request volume. Per-call usage is `MCP_TOOL_CALLED`.
+- **The MCP URL must be reachable without a redirect.** A cross-origin `301/302/307/308` strips the `Authorization` header in every spec-conforming client, and "cross-origin" includes the scheme — so a plain `http`→`https` canonicalisation at the proxy is as fatal as apex→www. It fails *loudly-looking-fine*: discovery is request-derived (`networkUtils.getRequestBaseUrl` reads `x-forwarded-proto`/host), so OAuth sign-in completes against the canonical origin while the client keeps POSTing the URL it was given, yielding permanent `401`s or a re-auth loop rather than a clean error. Activepieces never redirects there itself — the only prefixes are `/mcp` and `/mcp/platform`, and Fastify runs `ignoreTrailingSlash: true` so `/mcp/` matches the same route with no `301` — so it is always operator proxy config, and undetectable server-side (the proxy answers the pre-redirect request; AP never sees it).
 - OAuth discovery URLs are built via `domainHelper.getPublicUrlFromRequest` so subpath-hosted instances advertise the right prefix. `401`s carry an RFC 9728 `WWW-Authenticate: Bearer resource_metadata="…"` header. Host-root `.well-known/oauth-*` must still be forwarded to AP by the operator.
 - **DCR must issue a client secret when `token_endpoint_auth_method` is omitted.** RFC 7591 §2 says an omitted value defaults to `client_secret_basic`, *not* `none`, and [Microsoft Copilot Studio](https://learn.microsoft.com/en-us/microsoft-365/copilot/extensibility/plugin-authentication-dynamic-client-registration) refuses DCR outright without one ("DCR without a client secret isn't supported yet"). Defaulting an omitted method to `none` looks like it fixes the "public client handed a secret" contradiction, but it resolves it the wrong way: it breaks Copilot and makes `client_secret_basic` support unreachable for every client that omits the field. Resolve it the other way — default to `client_secret_basic` and keep issuing the secret.
 - `x-ap-conversation-id` header (EE chat) rebinds the server to a conversation's project, but only when scoping matches the token — it can never widen the grant.

brain/knowledge/engineering/architecture-spine.md

@@ -44,6 +44,8 @@ Activepieces: open-source AI-first workflow automation platform (self-hosted or
 
 **`unique()` from `core-utils` is O(n²) over `JSON.stringify` — never put it on a hot path.** It is `filter` + `findIndex` with a `JSON.stringify` on *both* sides of every comparison, so it blocks the event loop: 1k items → 42ms, 5k → 889ms, 10k → 3.6s, during which health checks, websockets and webhook dispatch all stall. It exists for deep-equality dedupe of objects; for primitives use `[...new Set(xs)]`. Found 2026-07 as the first statement of the bulk record delete the same PR was trying to speed up (GIT-1652).
 
+**`kebabCase()` from `core-utils` does not strip punctuation, so it cannot make a URL slug — reach for `slugify()`.** The two sit next to each other in `core-utils/utils.ts` and read as synonyms, but `kebabCase` only splits camelCase and swaps spaces/underscores for hyphens: `"Acme Inc."` comes back `acme-inc.`, dot intact, and any `&`, `'` or `/` survives too. `slugify` is the one that drops every non-alphanumeric run. Picking the wrong one is invisible in dev (single-word brand names are identical under both) and only shows up once a real customer name reaches the path, query string or config key you built with it. Neither has a fallback for an all-punctuation input — both return `''` — so a caller that needs a non-empty slug supplies its own default (`slugify(name) || 'activepieces'`, as the MCP client catalog does). Note `piece_set.key` is generated with `kebabCase`, which is deliberate: it is an opaque handle with a random suffix, not a URL.
+
 **`DeleteResult.affected` is `undefined` on PGlite — don't count rows with it.** TypeORM's `PostgresQueryRunner` only sets `affected` when the driver result carries `rowCount`; PGlite reports `affectedRows` instead and `typeorm-pglite` doesn't map it. So `result.affected ?? 0` is correct on `pg` and silently `0` on every PGlite deployment and test — the worst failure mode, since CI is green. Use `.returning('id')` and count the rows.
 
 **Migration timestamps are hand-picked, so two PRs in flight will collide.** `postgres-connection.ts` uses round numbers (`1815000000000`, `1816000000000`, …), not `Date.now()`, and TypeORM orders migrations by the 13-digit suffix of the class name. Two branches both taking "the next one" produce duplicate keys, and ordering — including `rollback-migrations.ts` — silently falls back to `getMigrations()` array order. Check `git ls-tree main packages/server/api/src/app/database/migration/postgres/` for the number before you commit, and re-check after any rebase.