[pull] main from activepieces:main

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

@@ -11,6 +11,7 @@ Exposes an Activepieces project as an MCP server so AI clients (Claude Desktop,
 **Grant** — one row of `mcp_oauth_token`: this user's live authorisation for one registered client. The unit the connect page lists and revokes, named `McpOAuthGrant` and served from `/v1/mcp-oauth/grants`.
 **Client** — one `mcp_oauth_client` registration row. Not a stable identity: Claude Code and Codex re-run DCR per sign-in, so one client-as-a-product yields many rows, and one user re-authenticating yields many grants. _Avoid_: using "client" for the thing being revoked.
 **Connection** — belongs to piece auth (`AppConnection`), never to MCP. _Avoid_: "MCP connection" in code; the tab label "Connections" and the `/mcp-server/connections` URL are deliberate copy, not the domain term — the code under `app/routes/mcp-server/grants/` says grant.
+**Pieces (tab)** — the piece actions a connected client can call in one project: the `/mcp-server/pieces` tab. Scoped to piece actions only, never the flow, table or run tools. "Reach" stays the *verb* the tab's own copy and the Connect and Connections copy use ("what it can reach", "the project it can reach") — it is not the label, because a one-word tab reads as a noun first and "Reach" names no object. The tab does link out to the piece-set admin page, so the label sits next to that page's vocabulary; that adjacency was judged the smaller cost. _Avoid_ as the label for this: "Reach" (retired), "Tools" (means the locked/controllable list in project settings), "Capabilities" (over-promises — implies the non-piece tools too), "Actions" (means flow steps), "Permissions" (RBAC, and nothing here is editable — the page is a mirror).
 
 ### Entities & services
 
@@ -64,6 +65,8 @@ Exposes an Activepieces project as an MCP server so AI clients (Claude Desktop,
 - **`openWorldHint` means the tool can change state in a third-party system**, not that it makes an outbound call. Anything that executes real connector steps needs it: `ap_test_flow`, `ap_test_step`, `ap_retry_run`, `ap_run_action`, and every dynamic flow tool. A read that only calls a connected account to populate dropdowns (`ap_get_piece_props`, `ap_resolve_property_options`, `ap_resolve_property_chain`) does not. `ap_retry_run` originally declared `false` here and was wrong — a retry re-runs the published flow and can resend the same Slack message or repeat an outbound write.
 - The hints are **advisory metadata for the client, never enforcement**. Authorization stays with `permissionChecker.wrapExecute` and each tool's `permission`; changing an annotation changes what a client is told, not what a caller is allowed to do.
 
+- **The Pieces tab's search is server-side, and it only works because `pieceDisplayName` is a Fuse key.** `/v1/pieces?searchQuery=` replaces each piece's `actions` with the matched subset (`searchForSuggestion`), which sounds fatal for a page that shows a per-piece action count and a destructive badge — but `searchForSuggestion` searches `['pieceDisplayName', 'displayName', 'description']`, so querying a *piece* name matches every action inside it and the row still lists the lot. Two more things make it safe: `toPieceMetadataModelSummary` computes `summary.actions` from the pre-search `audiencePieces`, so the total count is never narrowed by a query, and the tab force-expands every row while searching, so the count it renders is visibly the list beneath it. Keep the popular-first sort for the unsearched view only — applying it to search results throws away Fuse's relevance ranking. Rows are still grouped and counted client-side in `piecesUtils.toReachablePieces`, which is a pure function with its own unit test.
+
 ### Key files
 
 Entry point: `mcpServerModule`, the Fastify plugin in `mcp/mcp-module.ts` registered from `packages/server/api/src/app/app.ts`.
@@ -73,10 +76,19 @@ Entry point: `mcpServerModule`, the Fastify plugin in `mcp/mcp-module.ts` regist
 - `packages/server/api/src/app/mcp/oauth/` — OAuth 2.0 PKCE flow: metadata, authorize, token, revoke
 - `packages/core/shared/src/lib/automation/mcp/` — McpServer schema, McpToolDefinition, MCP OAuth types
 - `packages/web/src/app/components/project-settings/mcp-server/` — settings panel: credentials, flows-as-tools, tool toggles
+- `packages/web/src/app/routes/mcp-server/` — the Connect, Pieces and Grants tabs
 - `packages/web/src/app/routes/mcp-authorize/` — standalone OAuth consent page and its permission item
 - `packages/web/src/app/routes/embed/` — the `embedded-mcp-*` dialogs for managed-auth consent and settings
 - `packages/ee/embed-sdk/src/index.ts` — embed SDK public methods `authorizeMcp()`, `mcpSettings()`, `generateMcpToken()`
 - `packages/web/src/features/agents/agent-tools/` — adding an external MCP server as an agent tool
 - `packages/web/src/app/builder/test-step/custom-test-step/mcp-tool-testing-dialog.tsx` — test one MCP tool from the builder
 
 Paths verified 2026-07-17.
+- **Disabling `ap_run_action` leaves the catalogue fully browsable, and there is no way to hide it.** Piece
+  discovery (`ap_research_pieces`, `ap_search_actions`, `ap_search_triggers`, `ap_get_piece_props`) is in
+  `LOCKED_TOOL_NAMES`, which `disabledTools` cannot switch off — only the executor `ap_run_action` is
+  controllable. So a project that turns off running actions still lets a connected client enumerate every
+  piece and action it could theoretically call. That asymmetry is why the Pieces tab warns at the top of the
+  list rather than hiding the rows. Note the failure shape: a disabled tool is never `registerTool`d, so the
+  client gets an unknown-tool error from the protocol, not a permission denial from inside the tool — the
+  copy "every call fails" is directionally right but one layer off.

brain/knowledge/engineering/ci-pr-review-hygiene.md

@@ -20,6 +20,7 @@ Which team gets asked to review comes entirely from `.github/CODEOWNERS` — the
 Enforcement is the **`Codeowners review` repository ruleset** (active on the default branch), not classic branch protection: `require_code_owner_review: true` plus `required_approving_review_count: 1` and `required_review_thread_resolution: true`. Eight bypass actors are configured, which is why an owner-team request can look non-blocking on some PRs.
 
 ## Gotchas
+- **Never use `git stash` to prove a new test fails without its fix. Use `git checkout <base> -- <file>` instead.** `git stash push -- <path>` on a path with no uncommitted changes saves nothing and creates no entry, so a following `git stash pop` silently pops whoever's stash is at `stash@{0}` instead. This repo carries long-lived stashes from other branches, so the pop conflicts, is kept, and still writes that stash's untracked files into the working tree, which then look like your own new files. It has happened at least twice, and `stash@{1}` is literally named *"recovered: AGENTS.md agent-skills section (accidentally popped by claude)"*. Reverting one committed file to its base version and running the test there is the same proof with no shared state: `git checkout <merge-base> -- <file>`, run, then `git checkout HEAD -- <file>`. If a stash pop does go wrong, the entry survives the conflict, so the recovery is to delete the stray untracked files after confirming they belong to it with `git stash show --include-untracked --name-only stash@{0}`.
 - **Engine tests that call a live host are flakes waiting to happen, and the SSRF guard is off in tests so loopback is the fix.** `flow-rerun.test.ts` was the repo's top CI flake for months — two live calls to `cloud.activepieces.com` (a 404 plus `GET /api/v1/pieces`, the full catalog) inside a self-imposed 10s budget. It timed out 3× in one night on [#14966](https://github.com/activepieces/activepieces/pull/14966), a pieces-metadata-only PR, and 3 runs straight on [#14987](https://github.com/activepieces/activepieces/pull/14987), always within ~35ms of the limit; on a good day it merely *passed* at 8,163ms of 10,000ms. It was finally fixed by serving both responses from a `node:http` server on an ephemeral loopback port (8,163ms → 846ms), not by a bigger timeout — mid-investigation the host went fully unreachable, and no timeout value fixes a host that does not answer. Three facts that generalise: **(1)** `ssrfGuard`'s `isGuardEnabled` keys off `AP_NETWORK_MODE === STRICT`, which `packages/server/engine/vitest.config.ts` never sets, so the guard is inert in engine tests and a loopback server needs no config change — and `ssrf-guard.test.ts` passes explicit `allowList`s, so it is unaffected either way. **(2)** The engine's vitest default is already `testTimeout: 20000`; `flow-rerun` was the only file overriding it *downward*, which is why `flow-piece.test.ts` survived a 10,262ms call in the same run (it overrides *up* to 30s). Never override below the project default. **(3)** `piecePath.resolve` → `findInDistFolder` scans every dist `package.json` under `packages/pieces` (400+) on **every** call — only `pieceRunner.describe` results are cached, not the path — so the cold cost lands entirely in whichever test in a file runs first. That still applies to every other piece-loading engine test.
 - **Repo-wide regenerators sweep `main`'s pending drift into your PR — run them, then keep only your own lines.** `npm run i18n:extract` reorders all of `en/translation.json` and rewrites nine locale files (130 moved lines for six new keys), and `bun install` after a version bump writes back every community-piece version that was bumped without a lockfile sync (103 lines for four intended bumps). Both diffs are indistinguishable from real work in review, and both bury the change you actually made. Revert the file and hand-apply your own entries instead — then prove parity by running the generator into a scratch copy and diffing just your keys against it, so you keep byte-identical output without the churn. Provider setup markdown in `features/agents/ai-providers.ts` is extracted as translation keys in **source order**, so new entries go beside their neighbours in `SUPPORTED_AI_PROVIDERS`, not at the end.
 - **`.env.dev` is TRACKED, so the `.env*` line in `.gitignore` does not protect it — secrets put there get committed.** `.gitignore` line 82 is `.env*`, which reads as blanket protection for every env file, but gitignore has no effect on a path already in the index, and both `.env.dev` and `.env.example` are committed on `main`. `git check-ignore .env.dev` returns nothing, which is the tell. So an SMTP password or API key dropped into `.env.dev` shows up in `git status` as a normal modification and rides the next `git add -A`. Put local secrets under `dev/` instead — that whole directory is genuinely ignored (line 27) — and reach for `git check-ignore -v <path>` before writing a credential anywhere, rather than trusting the pattern.
@@ -32,6 +33,7 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def
 - **`redis-memory-server` compiles Redis from source during `bun install`, so its version must stay pinned.** It is in `trustedDependencies`, and with no version configured it defaults to `stable` — whatever `download.redis.io/redis-stable.tar.gz` points at today. When that moved to Redis 8.10.0 (2026-07-29), the bundled module tree (redisearch, redistimeseries, LibMR) started failing to build on runners and took `bun install` down across every branch: 8.10.0 vendors the module sources into the tarball and changes the default make goal to `build`, which compiles every module under `modules/*/src` regardless of `BUILD_WITH_MODULES`. It reads as flakiness because `ci.yml` caches `~/.bun/install/cache` but not the compiled binary, so each run recompiles and only sometimes survives. Root `package.json` pins `redisMemoryServer.version` to **8.8.1**, the newest release that still builds core-only — treat it as a ceiling, bump it deliberately, and never go back to `stable`.
 - **`validate-publishable-packages` compares against npm, not against `main`, so touching a published piece without bumping it fails CI on its own.** The error is `[packagePrePublishValidation] package version not incremented, path=packages/pieces/community/<piece>, version=X`. Editing *any* file in a published package is enough — a one-line change to the AI piece's model factory tripped it while `@activepieces/piece-ai` sat at `0.9.0` on npm. Check with `curl -s https://registry.npmjs.org/@activepieces/piece-<name> | jq -r ."dist-tags".latest`, and follow the piece's own history for the size of the bump: capability additions have gone minor, fixes patch. **The version lives in two files** — `package.json` *and* `bun.lock`, which records each workspace's version — so bump then `bun install`, or the lockfile check fails instead. Distinct from the merge-drift trap below: this one fires before any merge, and only for packages that are actually published.
 - **Standalone `prettier --check` disagrees with the `prettier/prettier` eslint rule in this repo, so it is a false guide — run `eslint` on the file.** From `packages/web`, `../../node_modules/.bin/eslint 'src/path/to/file.ts'` reproduces CI exactly and `--fix` resolves it. Standalone prettier flags files that are clean on `main` and that CI passes, whether invoked through `npx` or the pinned 2.8.4 with `--config .prettierrc` — so "prettier says it's unformatted" proves nothing, and chasing it wastes the time the eslint run would have taken. Only `packages/web` is prettier-enforced: the server and `packages/core/*` are 4-space, semicolon-free, and running prettier over them would rewrite the file wholesale.
+- **Linting a single server test file OOMs node at its default heap — pass `NODE_OPTIONS=--max-old-space-size=8192`.** `npx eslint packages/server/api/test/.../<file>.test.ts` on one file died with `FATAL ERROR: Reached heap limit` after ~23s at 2GB, because the type-aware config loads the whole `packages/server/api` program regardless of how few files you name. It reads as a broken lint setup, not as a memory ceiling. The same run with an 8GB heap finishes and reports normally.
 - **`bun install` on a recent bun adds `"configVersion": 0` to `bun.lock`, which is not on `main`.** It rides along in any commit that touches the lockfile and reads as an unrelated change; drop the line and re-run `bun install --frozen-lockfile` to confirm the lockfile is still consistent without it.
 - **A version bump that merges cleanly can still be wrong — check what `main`'s number *means*, not whether it conflicts.** Two branches bumping the same package to the same number do not conflict, so git takes it silently; but if `main`'s copy of `0.5.0` is another PR's content and yours adds further exports on top, you ship new exports under an already-published version and nothing catches it. Seen merging [#15001](https://github.com/activepieces/activepieces/pull/15001) after the six-providers PR landed: `core-piece-types` and `pieces-framework` auto-merged at `0.5.0` / `0.37.0` and both needed a further bump. Only a *conflicting* version (like `core/shared` `0.140.0` vs `0.141.0`) forces you to think; the clean ones are the dangerous ones. After any merge, re-check every package you bumped against `git show origin/main:<pkg>/package.json`. The reverse also happens: when review makes you *delete* code, the bump it justified can become dead — after acting on review, `git diff origin/main...HEAD -- <pkg>/src` and drop the bump if it is empty. On #15001 two packages ended up byte-identical to `main` while still carrying a bump, which is noise at best and a version collision at worst.
 - **`@activepieces/shared` re-exports from `@activepieces/core-execution`, so a partial rebuild produces phantom "has no exported member" errors in unrelated files.** Rebuilding `core/shared` against a stale `core/execution` dist drops those re-exports, and the API typecheck then fails in `ee/agent/*` on symbols like `GetPersonalizationConfigRequest` — which live in `core/execution/src/lib/workers/worker-contract.ts`, not in shared at all. It reads exactly like a bad merge. The dependency order that actually works is `core/utils` → `core/piece-types` → `core/formula` → `core/execution` → `core/shared` → `server/utils` → `pieces/framework` → `core/ai-providers`; skipping a link silently poisons everything downstream of it. The same staleness makes an editor report missing enum members that exist in the source.
@@ -50,3 +52,4 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def
 - **A branch that predates the `brain/` → `brain/knowledge/` move cannot edit a brain page in place — GitHub will call the PR conflicting even when `git merge` is clean locally.** Git follows the rename and merges the modification into the new path; GitHub's mergeability check does not, so it reports `modify/delete` on the old path and the PR goes `dirty`. Local `git merge-tree --write-tree` exits 0 and hides the problem; reproduce what GitHub sees with `git merge -X no-renames origin/main`. Fix: merge `origin/main` into the branch first, which lands the edit at the new path, then push.
 - **`breaking-change-check` couples the docs entry to the label in BOTH directions, so back-documenting an already-shipped change drags the label onto a docs-only PR.** R3 in `tools/scripts/breaking-change-check.ts` fails a PR that adds a `####` entry to `docs/install/reference/breaking-changes.mdx` without `⛓️‍💥 breaking-change`, exactly as it fails the label without an entry — and the template answer has to agree too, so "yes" must be ticked on a PR that changes no code. It reads the *added lines of that one file* from `git diff origin/<base>...HEAD`, and `hasBreakingEntry` wants a `####` heading **plus** a non-heading body line, so a heading alone, a `---`, or a version bump does not count. Two consequences: the label then collides with `skip-changelog` in release-drafter (pick one deliberately — the feature's own PR usually already carried the changelog entry), and an entry appended to a *released* section still trips it, since the check never looks at which heading the lines landed under.
 - **Nothing rolls `## Unreleased` over at release time, and the docs site is unversioned — so a breaking-changes entry has to name its own version.** No workflow or script writes to `docs/install/reference/breaking-changes.mdx` (`breaking-change-check.ts` only reads it), and `git log -S"## 0.88"` on the file comes back empty: the heading has not moved since 0.87.0, so entries for work that shipped months ago still sit under "Unreleased" (PM2 removal in 0.88.2, cache pre-warm gate and workspace naming in 0.89.0, …). `docs/docs.json` has no versioning either, so there is one live page for every self-hoster whatever version they run, published on merge rather than on release — the version heading is the *only* thing telling a reader whether a change is already in their build. So before adding an entry, run `git tag --contains <commit>` on the change it describes and file it under the release that actually shipped it; only genuinely unshipped work belongs under "Unreleased". What points self-hosters at the page in the first place is `release-drafter.yml`, which appends a "review the Breaking Changes page" line to every release body and groups `⛓️‍💥 breaking-change` PRs under their own heading — which also means a docs-only PR back-documenting an old change shows up in the *next* release's breaking-change list.
+- **Greptile enforces the file-order rule on *private* constants too, which CLAUDE.md only states for exported ones.** CLAUDE.md says "Exported types and constants must be placed at the end of the file" and gives the order as imports → exports → helpers → types; Greptile reads that as covering module-private constants as well, and flags a `const` sitting above the file's exported symbol (P2 on [#15226](https://github.com/activepieces/activepieces/pull/15226), for two constants only read inside the service they sat above). It has that as a stored custom-context memory, so it will keep raising it. Put private constants in the helpers section below the export — hoisting is a non-issue when they are only read at call time.