From a3d1466b1c85643d0cdd38013cc96815f762442a Mon Sep 17 00:00:00 2001 From: Othman Abu Ajamieh <52608229+othmanemad@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:07:33 +0300 Subject: [PATCH 1/3] feat(mcp): Pieces tab in the MCP page showing available pieces (#15080) Co-authored-by: Abdul <106555838+AbdulTheActivePiecer@users.noreply.github.com> --- brain/knowledge/ai-intelligence/mcp-server.md | 12 + .../engineering/web-feature-anatomy.md | 7 +- brain/knowledge/pieces-engine/piece-sets.md | 6 +- brain/knowledge/pieces-engine/pieces.md | 1 + .../metadata/piece-metadata-controller.ts | 30 +- .../ce/pieces/piece-metadata.test.ts | 14 + .../pieces/piece-component-filtering.test.ts | 68 ++++- packages/web/AGENTS.md | 1 + .../web/public/locales/en/translation.json | 19 ++ .../web/src/app/routes/mcp-server/index.tsx | 11 +- .../web/src/app/routes/mcp-server/mcp-nav.ts | 10 +- .../routes/mcp-server/pieces/piece-row.tsx | 158 ++++++++++ .../routes/mcp-server/pieces/pieces-tab.tsx | 289 ++++++++++++++++++ .../routes/mcp-server/pieces/pieces-utils.ts | 89 ++++++ .../mcp-server/pieces/project-picker.tsx | 71 +++++ .../src/features/pieces/hooks/pieces-hooks.ts | 62 +++- .../src/features/pieces/hooks/steps-hooks.ts | 2 +- packages/web/src/lib/api.ts | 4 +- .../mcp-server/pieces/pieces-utils.test.ts | 167 ++++++++++ .../features/pieces/pieces-hooks.test.tsx | 116 +++++++ 20 files changed, 1116 insertions(+), 21 deletions(-) create mode 100644 packages/web/src/app/routes/mcp-server/pieces/piece-row.tsx create mode 100644 packages/web/src/app/routes/mcp-server/pieces/pieces-tab.tsx create mode 100644 packages/web/src/app/routes/mcp-server/pieces/pieces-utils.ts create mode 100644 packages/web/src/app/routes/mcp-server/pieces/project-picker.tsx create mode 100644 packages/web/test/app/routes/mcp-server/pieces/pieces-utils.test.ts create mode 100644 packages/web/test/features/pieces/pieces-hooks.test.tsx diff --git a/brain/knowledge/ai-intelligence/mcp-server.md b/brain/knowledge/ai-intelligence/mcp-server.md index 20b9916bc108..2dd02d8a69f9 100644 --- a/brain/knowledge/ai-intelligence/mcp-server.md +++ b/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,6 +76,7 @@ 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()` @@ -80,3 +84,11 @@ Entry point: `mcpServerModule`, the Fastify plugin in `mcp/mcp-module.ts` regist - `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. diff --git a/brain/knowledge/engineering/web-feature-anatomy.md b/brain/knowledge/engineering/web-feature-anatomy.md index 365580358d10..cdbac5e38c39 100644 --- a/brain/knowledge/engineering/web-feature-anatomy.md +++ b/brain/knowledge/engineering/web-feature-anatomy.md @@ -66,6 +66,7 @@ Verify with `npx turbo run lint --filter=web`, or `npm run lint-dev` for the who - **A panel that hand-rolls its draft state gets none of the form validation the rest of the app assumes.** react-hook-form + `zodResolver` is what surfaces `formErrors.required` and friends; a `useState` draft with a Save button has no schema, so the usual mistake is to *substitute* a fallback for an empty field (`name.trim().length > 0 ? name.trim() : existing.name`) instead of rejecting it. That reads as a silent failure: the request succeeds, the old value returns, and nothing explains why. When a surface cannot use react-hook-form, derive the invalid state, render the message next to the field, and disable the submit — do not paper over the empty value. Bit the AI Center key-detail panel while its sibling connect dialog, on a zod resolver, was correct. The second failure mode is that such a draft never resyncs: seeded once from a prop, it outlives any refetch of the row it mirrors, so a mutation that changes the row without changing its `key` (the AI Center replaces a key's credentials, and the panel is keyed on the config id) leaves the draft describing the old row — phantom "unsaved changes", and a save that reverts what the mutation just wrote. Bump a version segment into the `key` at the site that performs the mutation rather than diffing props inside the panel: TanStack Query hands back a new object identity on every refetch, so a naive identity comparison discards the admin's unsaved edits on a window refocus. - **Exported types and constants belong at the *end* of the file**, after the components and logic. Reading a file should start with what it does, not its type declarations. - **`showErrorDialog` on the wrong query is worse than missing it.** On an auxiliary query it throws a modal over a page that was working fine; on the primary query, omitting it leaves the user staring at an empty table with no explanation. +- **A `data ?? []` default turns a failed query into an empty state, and `showErrorDialog` does not cover for it.** The modal is page-independent (`QueryCache.onError`), so the body still renders "nothing here" underneath it — and its copy ("your data is safe, refresh the page") is wrong for a permission denial the user caused by editing a URL param. When a page can render its own inline error state, branch on `isError` *before* the empty state and drop `showErrorDialog` on that query rather than showing both. `api.isApError(error, ErrorCode.X)` is how you tell an access denial apart from a network blip — note it reads the *response body's* `code`, so it needs the server's `ActivepiecesError` code, not an HTTP status. - **A ref assigned during render (`const ref = useRef(x); ref.current = x`) is stale inside socket/event callbacks.** The value only advances when React commits a render, so two events handled before that commit both read the same base — a read-modify-write (merging a step into `run.steps`) silently drops the earlier event. Read the zustand store directly instead: `useBuilderStore().getState()` (`app/builder/builder-hooks.ts`) always returns current state. Bit the test-flow widget's progress merge, PR #14453. - **Builder overlays share one stacking context, so a big `z-` wins over everything — including portalled popovers.** Nothing between an overlay in the canvas panel and `` creates a stacking context (the middle panel is `relative` + `z-auto`; `ResizablePanel` sets only flex/overflow), so a canvas child's `z-index` competes directly with Radix portals. The working ladder: canvas `z-30` (opaque `bg-builder-background` — anything below it is invisible), header and floating corner chrome `z-40`, data selector / canvas controls / popovers `z-50`. That is why the powered-by note at `z-10000` painted over the piece selector. - **The flow "download as image" only captures `.react-flow__viewport`.** `flowScreenshotUtils` (`flow-canvas/utils/flow-screenshot-utils.ts`) clones that one element into an SVG, so anything outside it — the dot-grid background, the powered-by note, canvas controls — is absent unless handled explicitly. Two seams: mark in-viewport chrome you want *omitted* (step chevron, badges) with `data-flow-screenshot-exclude`; anything *outside* the viewport you want *included* has to be redrawn onto the composited 2D canvas in `composeImageWithCanvasBackground` (that's how the background dots and the powered-by mark get there). @@ -79,7 +80,11 @@ Verify with `npx turbo run lint --filter=web`, or `npm run lint-dev` for the who - **`packages/web`'s lint script only globs `src/**`, so nothing under `packages/web/test/` is ever linted** — not by CI's `lint` job, not by `npm run lint-dev`. Running `npx eslint 'test/**/*.{ts,tsx}'` from `packages/web` today reports 21 errors nobody has seen, so a new web test needs a manual eslint pass or it ships with errors. Most common trap: `testing-library/render-result-naming-convention` fires on any local helper whose name merely *starts with* `render` even when testing-library is not involved — renaming `render` to `renderTabText` does not silence it, only a name that doesn't begin with `render` does. - **`AllowOnlyLoggedInUserOnlyGuard` calls its hooks after two early returns, and the linter only lets it.** `react-hooks/rules-of-hooks` does not flag member-expression calls, so `platformHooks.useCurrentPlatform()` / `flagsHooks.useFlags()` sail past it — but add a bare `useSomething()` there and the rule fires, correctly: `isLoggedIn()` can change between renders, so those calls really are conditional. Anything new that needs to run once a session is authenticated belongs in a null-rendering component placed inside the returned `` subtree, which mounts only after the guard passes. That is why automatic trial activation is `` and not a hook. - **The layering is lint-enforced, not just a convention.** `packages/web/.eslintrc.json` has an `import/no-restricted-paths` zone making the codebase unidirectional: `src/app` may import `src/features`, and both may import `src/lib`/`hooks`/`components`/`types`/`utils` — never the reverse (the one exception is `app/query-client.ts`). So a hook that a public route needs belongs in `src/lib`, but anything rendering a feature's components has to live in that feature; you cannot keep the pair in one `lib` file. It fails as an `import/no-restricted-paths` **error**, not a warning, so it blocks lint. -- **Arbitrary Tailwind values for type, tracking and radius get sent back in review — `packages/web` has its own scale and it is not stock Tailwind.** There is no `tailwind.config.js`; this is Tailwind v4 and the theme lives in the `@theme` block of `src/styles.css`, which *adds* `--text-xss: 0.65rem`, *overrides* `--text-3xl` to 1.75rem and `--text-4xl` to 2rem (both smaller than stock), and derives `--radius-{sm,md,lg,xs,xss}` from a single `--radius: 0.5rem`. So `text-[13px]`, `tracking-[-0.025em]` and `rounded-[11px]` are not just style nits — they sit *between* real tokens and drift the page off the scale. Map them: 10–11px → `text-xss`, 11.5–12.5px → `text-xs`, 13–13.5px → `text-sm`, 15–15.5px → `text-base`; negative tracking → `tracking-tight`, uppercase-eyebrow tracking → `tracking-wide`/`wider`; any `rounded-[9–11px]` → `rounded-md`. Layout constraints are the exception and stay arbitrary — `max-w-[628px]` for a reading measure or `lg:w-[344px]` for a sidebar have no token equivalent and are idiomatic. Fractional spacing (`size-5.5`, `size-8.5`, `size-13`) is valid in v4 and beats `size-[22px]`. Neither eslint nor `tsc` catches any of this, so it only ever surfaces in review. +- **Arbitrary Tailwind values for type, tracking and radius get sent back in review — `packages/web` has its own scale and it is not stock Tailwind.** There is no `tailwind.config.js`; this is Tailwind v4 and the theme lives in the `@theme` block of `src/styles.css`, which *adds* `--text-xss: 0.65rem`, *overrides* `--text-3xl` to 1.75rem and `--text-4xl` to 2rem (both smaller than stock), and derives `--radius-{sm,md,lg,xs,xss}` from a single `--radius: 0.5rem`. So `text-[13px]`, `tracking-[-0.025em]` and `rounded-[11px]` are not just style nits — they sit *between* real tokens and drift the page off the scale. Map them: 10–11px → `text-xss`, 11.5–12.5px → `text-xs`, 13–13.5px → `text-sm`, 15–15.5px → `text-base`; negative tracking → `tracking-tight`, uppercase-eyebrow tracking → `tracking-wide`/`wider`; any `rounded-[9–11px]` → `rounded-md`. Layout constraints are the exception and stay arbitrary — `max-w-[628px]` for a reading measure or `lg:w-[344px]` for a sidebar have no token equivalent and are idiomatic. Fractional spacing (`size-5.5`, `size-8.5`, `size-13`) is valid in v4 and beats `size-[22px]`. Neither eslint nor `tsc` catches any of this, so it only ever surfaces in review — the mapping is written up in the *Tailwind / Styling* section of `packages/web/AGENTS.md` so agents meet it before writing the class. Above 15.5px there is no px mapping, because heading sizes are a per-surface decision: copy the token the neighbouring heading on the same page already uses (a `text-[22px]` page heading becomes the `text-xl` its sibling section headings use) rather than rounding to the closest number. - **`npx prettier --check` lies about `packages/web` — it flags files nobody has touched, so never treat it as a gate.** Prettier is not in any CI workflow, and the root `.prettierrc` is a single `{"singleQuote": true}` while the resolved binary is prettier **2.8.4**, whose `trailingComma` default is `es5`. The checked-in code is formatted by prettier 3 (via the editor / eslint integration), which defaults to `all` — so every multi-line call with a trailing comma reads as a "code style issue". Running `--check` on a file straight out of `git show HEAD:` reproduces it. If you want to know whether your own edit is formatted, diff `npx prettier ` against the file and check the hunks are yours; the pass/fail verdict is meaningless. `npx turbo run lint --filter=web` is the real gate. - **A date test with hardcoded `Z` fixtures is a false green — CI runs UTC, and both `dayjs().isSame(x, 'day')` and `formatUtils.formatDate` are *local*.** Freezing the clock with `vi.setSystemTime(new Date('…Z'))` and then asserting against a literal `'2025-09-15T00:30:00Z'` only holds where local time is UTC. `grant-utils.test.ts` on [#15079](https://github.com/activepieces/activepieces/pull/15079) was 3/3 green in CI and on `TZ=UTC`, 1 failed on `TZ=America/New_York` (`00:30Z` is the *previous* local day, so "Active today" flips to "Last used Yesterday"), 2 failed on `TZ=Pacific/Honolulu` (the second being `formatDate` rendering `Aug 11` where the test asserted `Aug 12`). Nobody in the Americas can run the suite clean, and nothing in CI will ever tell you. Derive every fixture from the frozen clock instead of writing a literal — `dayjs(NOW).startOf('day').add(30, 'minute')`, `dayjs(NOW).subtract(34, 'day')` — and assert with the same local formatter the code uses (`earlier.format('MMM D')`), so fixture and assertion move together in any zone. Check any new date test with `TZ=America/New_York` and `TZ=Pacific/Honolulu` before pushing; those two straddle UTC on both sides and catch it. The production `isSame(…, 'day')` is *correct* — a user's "today" is their own day — so the bug is always in the test, never in the formatter. - **`ConfirmationDeleteDialog`'s `entityName` is a required prop that renders nowhere unless you also pass `showToast` — 25 of its 30 call sites compute a label and throw it away.** `components/custom/delete-dialog.tsx` mentions `entityName` three times: the prop type, the destructure, and one `toast.success(t('Removed {entityName}', …))` sitting inside `if (showToast)`. `showToast` is optional and there is no default, so every caller that omits it (or passes `false`) gets no toast and no other use of the value. The dialog body renders `title` and `message` only, so the confirmation never names what is about to be deleted. `project-member-card.tsx` builds `` `${firstName} ${lastName}` `` for nothing; `api-keys/index.tsx` passes `t('API Key')` for nothing. Nothing catches it — the prop is required, so TypeScript is satisfied, and lint has no opinion. Caught on [#15079](https://github.com/activepieces/activepieces/pull/15079), where it also made a newly added `revokedGrants` ICU plural rule unreachable in every locale — a dead translation key that `i18n:extract` will happily keep regenerating. When you want the name on screen, interpolate it into `message` yourself (`t('Revoking {entityName}. …', { entityName: label })`); passing `entityName` alone does nothing. Before adding a translation key for a dialog label, grep for where the prop you are feeding actually renders. +- **A `bg-muted/40` panel is `#FBFBFB`, not `#F5F5F5` — an alpha wash over white is far lighter than the token it names, so a Paper mock's flat grey slab is not what the code renders.** `--muted` is `neutral-100` (`#F5F5F5`); at 40% over a white card that resolves to about `#FBFBFB`, a shade nobody would call grey. This matters when a design review compares a mock to the app: the MCP Pieces action panel *looked* like low-contrast text on a grey ground in Paper, while the shipped panel was already near-white and its real contrast problem was elsewhere (10px labels, a count at `--color-text-faint` on the mock's grey ≈ 2.3:1). Resolve the alpha before concluding anything about contrast, and prefer stating the computed hex in review. Related: for a *tinted* pill or frame, reach for the `Badge` variants (`destructive` / `warning` / `success` / `info` in `components/ui/badge.tsx`) rather than hand-rolling `bg-*-50 text-*-700` — each already carries the matching `dark:bg-*-950 dark:text-*-300` pair, which you have to write yourself otherwise because the numbered scales are not redefined in the `.dark` block. +- **Two everyday building blocks carry a hidden per-instance cost, so a long list gets expensive well before anyone notices — and `VirtualizedList` only helps if the list has a scrollable ancestor.** `TextWithTooltip` registers its own `window` resize listener and does a `scrollWidth`/`clientWidth` layout read per instance (`components/custom/text-with-tooltip.tsx`), and `PieceIcon` renders `ImageWithColorBackground`, which fetches the logo *and* runs `fast-average-color` `getColorAsync` on it, then sets state two or three times (`components/custom/image-with-color-background.tsx`). A row using one icon and two tooltips therefore costs an image fetch, a canvas colour extraction and two resize listeners; the MCP Pieces list hits ~740 rows behind its "Show N more" button, which is a thousand-plus listeners and as many colour extractions from one click. Reach for `components/ui/virtualized-list.tsx` (already on `@tanstack/react-virtual`) rather than rolling one: its `virtualizeThreshold` defaults to 100 so a short list keeps its plain inline render, and it measures rows with `measureElement`, so variable heights work. The prerequisite is that `findScrollParent` locates an `overflow: auto|scroll` ancestor or a Radix `data-slot="scroll-area-viewport"` — dashboard pages get one from `app/components/project-layout` — because with no scroll element the virtualizer keeps its seeded viewport and never responds to scrolling. Note also that virtualization does nothing for a *re-render* storm: deriving rows in the render body (filter + sort + fresh objects) and un-memoised rows re-do that work on every keystroke, which is a `useMemo`/`memo` problem, not a windowing one. +- **Virtualizing an existing list silently kills every `:last-child` style on its rows.** `VirtualizedList` wraps each row in its own absolutely-positioned element, so a row that was one of many siblings becomes an only child — `last:border-b-0`, meant to drop the final separator, then matches *every* row and removes all of them. Nothing errors and the list still renders; the borders are just gone. Its non-virtualized branch wraps items in fragments, which create no DOM, so `:last-child` keeps working below the threshold and the two paths disagree — the bug appears only once the list crosses 100 items. Make the separator explicit (pass the row its index or an `isLastRow` flag) before wrapping an existing bordered list, and check the same styles for `:first-child`, `:nth-child` and sibling combinators like `space-y-*`. +- **`placeholderData: keepPreviousData` reuses the last result on *any* query-key change, so on a scoped query it renders one scope's data under another scope's label.** It is reached for to stop a debounced search flashing a skeleton on every settle, which is the transition it earns its keep on — but the key usually carries a scope segment too (a `projectId`), and switching that is treated identically. The MCP Reach tab showed the previous project's pieces under the newly picked project until the replacement landed, and for a project the user cannot see, until the denial swapped in the access alert. Nothing crosses a permission boundary (the rows were fetched and authorised under the previous scope, and the pending request can only return the new scope's data or a denial) so it is misattribution, not disclosure — but on a page whose claim is "this is what a client can reach in *this* project", an admin reads a restricted project as wide open. Scope the placeholder instead of dropping it: the v5 form takes a second argument, so `(previousData, previousQuery) => previousQuery?.queryKey[SCOPE] === scope ? previousData : undefined` keeps the search behaviour and restores the loading state on a scope switch, where it is the correct feedback anyway. Put the scope segment early in the key so the comparison is stable, and cover it with a test — the positional index is exactly what a later key reorder breaks silently. A filter-driven list (a multi-select of projects in the URL, as on the Connections tab) is *not* the same case and wants the plain `keepPreviousData`. diff --git a/brain/knowledge/pieces-engine/piece-sets.md b/brain/knowledge/pieces-engine/piece-sets.md index 94b496626604..06eab0d512d7 100644 --- a/brain/knowledge/pieces-engine/piece-sets.md +++ b/brain/knowledge/pieces-engine/piece-sets.md @@ -23,9 +23,13 @@ A named, reusable piece/action/trigger visibility configuration a platform admin - The **whole** `/v1/piece-sets` module is behind that flag, `GET` included — so on a locked plan the web list query is `enabled: false`, the table is simply empty, and row actions never render. Only toolbar/entry points need a UI guard. The `LockedAlert` + `RequestTrial featureKey="ENTERPRISE_PIECES"` lives once on `PlatformPiecesPage`, above the tabs, since the same flag gates both the Pieces and Piece Sets tabs; the details route redirects back to the tab rather than hanging on a spinner waiting for a query that will never run. - There is **no** install-time sync and no `onPieceCreated` hook — resolution is purely read-time. See ADR 0001 (visibility derived, not materialized). - Embed auth: a v4 JWT carries a `pieceSet` key claim; legacy v2/v3 tokens carry `piecesTags` (only the first tag honored, resolved to `key = tag`, else Default). Enforcement (`applyProjectPieceAccess`) runs unconditionally, not gated by the flag. -- **`usePieces({ skipProjectFilter: true })` is not a caching flag — it silently turns piece-set filtering off.** It drops `projectId` from `GET /v1/pieces`, and `resolveVisibility` (`ee/pieces/filters/piece-filtering-utils.ts`) bails to `null` the moment *either* `platformId` or `projectId` is nil, so the response is the unfiltered platform catalog. `platformId` still comes from the principal, so this is not a tenancy hole — but any surface using it advertises pieces a restricted project's flows and MCP server will not actually expose. Correct for platform-admin screens (the piece-set editor has to list pieces you have not permitted yet) and for a marketing-style showcase; wrong anywhere the list implies "what you can use here". The absence of `projectId` is easy to miss at the call site because the flag reads like a client-side concern. +- **`usePieces({ skipProjectFilter: true })` is not a caching flag — it silently turns piece-set filtering off.** It drops `projectId` from `GET /v1/pieces`, and `resolveVisibility` (`ee/pieces/filters/piece-filtering-utils.ts`) bails to `null` the moment *either* `platformId` or `projectId` is nil, so the response is the unfiltered platform catalog. `platformId` still comes from the principal, so this is not a tenancy hole — but any surface using it advertises pieces a restricted project's flows and MCP server will not actually expose. Correct for platform-admin screens (the piece-set editor has to list pieces you have not permitted yet) and for a marketing-style showcase; wrong anywhere the list implies "what you can use here". The absence of `projectId` is easy to miss at the call site because the flag reads like a client-side concern. The mirror-image trap: with the flag off, `usePieces` scopes to `authenticationSession.getProjectId()` — the *session's* project — so a platform-admin screen inspecting some other project (the MCP Reach tab, with its project picker) must pass `projectId` explicitly or it will quietly render the admin's own project's pieces under another project's name. - Migration is three ordered steps: create table + backfill (`1807...`), then `CREATE INDEX CONCURRENTLY` (`1808...`, non-transactional), then the breaking drop of legacy platform piece-filter columns (`1809...`). Legacy `tag`/`piece_tag` tables are kept only because the backfill reads them once via raw SQL. +- The three `GET /v1/pieces*` routes are `securityAccess.unscoped(ALL_PRINCIPAL_TYPES)` but accept a `projectId` **query param** that picks which project's piece set filters the result — the route security does not scope it. The handlers assert membership themselves via `rbacService.assertPrinicpalAccessToProject` (membership only, no permission, skipped for principals with no platformId since visibility is already inert for them). Any new route that takes `projectId` for visibility must do the same: `resolvePieceSetForProject` looks the project up by id alone. That assertion carries the same two carve-outs `resolveVisibility` needs, both pinned by tests. It is **edition-gated** to EE/Cloud: `projectId` reaches *nothing* but `resolveVisibility` (not the search or sort path), so on CE the param is inert and an ungated membership check could only turn a working 200 into a 403/404. And it skips an **empty** `projectId` as well as a nil one, because `isNil('')` is false and `''` would otherwise reach `projectService.getOneOrThrow('')` and 404 — the web can produce exactly that, since `qs.stringify` serializes a null `projectId` as `projectId=` (so pass `getProjectId() ?? undefined`, never `getProjectId()!`). What remains: on EE/Cloud a nonexistent or soft-deleted project id answers 404 while a real project you are not a member of answers 403, which is a project-existence oracle for any authenticated user. +- **What each principal actually gets from `GET /v1/pieces?projectId=`** (measured on all three editions, all three routes — they never diverge). A **project member of any role** (VIEWER included) reads its own project and is refused a sibling with 403; a **platform ADMIN or OPERATOR** reads every project on its platform through the implicit role `projectMemberService.getRole` grants; a **SERVICE api key** reads every project on its own platform and is refused another platform's. **WORKER, UNKNOWN and unauthenticated callers are skipped and leak nothing** — with any `projectId` they get the *unfiltered platform catalogue*, exactly as if the param were absent, because `resolveVisibility` bails on a nil `platformId`. So they also never receive filtering: a piece a project's set hides is still visible to them. **ONBOARDING** never reaches these handlers at all (401 `INVALID_BEARER` at authentication), so the ONBOARDING arm of `getPlatformId` is dead code here. **ENGINE** is refused anything but its own `projectId`, a nonexistent id included, since that arm compares ids without a lookup — no caller does this today, but it is a trap for the first one that tries. +- Because the guard makes these routes *able* to fail, any surface that puts a user-controlled `projectId` on them has to surface the denial. The Reach tab does not yet: a `?project=` the caller cannot read answers 403 (or 404 for an unknown id) and the page renders its **"No pieces are reachable in this project."** empty state with no error, which reads as "this project has no pieces" rather than "you have no access" — verified against a live EE server, and not a stale bundle or a missing `showErrorDialog`. +- Embed tenants are isolated from each other's piece sets: a token minted through `POST /v1/managed-authn/external-token` reads its own project, and is refused both a sibling project and another embed user's project with 403. That endpoint is a convenient way to get a *real* embed principal in a test, rather than hand-rolling one. ### Key files Entry point: `pieceSetService`, defined in `piece-set.service.ts` and wired to the `/v1/piece-sets` routes by `piece-set.controller.ts`. diff --git a/brain/knowledge/pieces-engine/pieces.md b/brain/knowledge/pieces-engine/pieces.md index 0486b0ede17b..95aed9b6a5b8 100644 --- a/brain/knowledge/pieces-engine/pieces.md +++ b/brain/knowledge/pieces-engine/pieces.md @@ -28,6 +28,7 @@ The metadata catalog of automation integrations ("pieces") — each a named inte - `DynamicPropertiesContext` tracks loading by property name only, so two in-flight requests for the same property let the first completion clear the flag for both — briefly re-enabling Test Step while the value is still cleared. - **The frontend `POST /v1/pieces/options` client only rejects for DYNAMIC.** `piecesApi.options` (`packages/web/src/features/pieces/api/`) catches DROPDOWN failures, toasts, and *resolves* with a disabled-dropdown fallback — so for dropdowns every error path wired onto that mutation is dead: `usePieceOptions`' `onError` handlers, its `retry: 1`, and the `if (error) throw error` into `DynamicPropertiesErrorBoundary`. DYNAMIC must rethrow: a swallowed failure arrives as a *successful* empty schema, which resets the property's children to defaults and gets persisted by step-settings autosave. - **`AP_DEV_PIECES` shadows the DB registry copy by name**, so a dev piece failing the release gate removes the piece *entirely* rather than falling back to the published version. Dropping the name from `AP_DEV_PIECES` (or bumping the local root `package.json`) brings it back. +- **A piece search narrows `suggestedActions` to the actions that matched — and matching the *piece* name matches all of them.** `pieceSearching.search` (`pieces/metadata/utils/piece-searching.ts`) runs Fuse over the pieces, then re-runs a nested Fuse per hit through `searchForSuggestion` and returns only the matching actions/triggers. That nested search includes `pieceDisplayName` in its keys and stamps it onto every action, so querying "slack" scores every Slack action as a suggestion, while "archive channel" returns a short list. So `suggestedActions` on a search response is *the answer to the query*, not the piece's full catalogue — a UI that expands search results is showing what matched, and one that caches them must key on the query. Without a `searchQuery` the field is the normal suggestion set instead. ### Key files Entry point: `pieceModule`, the Fastify plugin registered in `packages/server/api/src/app/app.ts` that mounts every `/v1/pieces` route. diff --git a/packages/server/api/src/app/pieces/metadata/piece-metadata-controller.ts b/packages/server/api/src/app/pieces/metadata/piece-metadata-controller.ts index 94ccf91491dc..eeac17ff19e5 100644 --- a/packages/server/api/src/app/pieces/metadata/piece-metadata-controller.ts +++ b/packages/server/api/src/app/pieces/metadata/piece-metadata-controller.ts @@ -1,14 +1,17 @@ -import { ActivepiecesError, ErrorCode, isNil, LocalesEnum } from '@activepieces/core-utils' +import { ActivepiecesError, ErrorCode, isEmpty, isNil, LocalesEnum } from '@activepieces/core-utils' import { PieceMetadataModel, PieceMetadataModelSummary } from '@activepieces/pieces-framework' -import { ALL_PRINCIPAL_TYPES, EngineResponse, GetPieceRequestParams, GetPieceRequestQuery, GetPieceRequestWithScopeParams, ListPiecesRequestQuery, PieceAudienceFilter, PieceCategory, PieceOptionRequest, Principal, PrincipalType, RegistryPiecesRequestQuery, SampleDataFileType, WorkerJobType } from '@activepieces/shared' +import { ALL_PRINCIPAL_TYPES, ApEdition, EngineResponse, GetPieceRequestParams, GetPieceRequestQuery, GetPieceRequestWithScopeParams, ListPiecesRequestQuery, PieceAudienceFilter, PieceCategory, PieceOptionRequest, Principal, PrincipalType, RegistryPiecesRequestQuery, SampleDataFileType, WorkerJobType } from '@activepieces/shared' +import { FastifyBaseLogger } from 'fastify' import { FastifyPluginAsyncZod } from 'fastify-type-provider-zod' import { StatusCodes } from 'http-status-codes' import { z } from 'zod' import { ProjectResourceType } from '../../core/security/authorization/common' import { securityAccess } from '../../core/security/authorization/fastify-security' +import { rbacService } from '../../ee/authentication/project-role/rbac-service' import { resolveVisibility } from '../../ee/pieces/filters/piece-filtering-utils' import { flowService } from '../../flows/flow/flow.service' import { sampleDataService } from '../../flows/step-run/sample-data.service' +import { system } from '../../helper/system/system' import { userInteractionWatcher } from '../../workers/user-interaction-watcher' import { pieceSyncService } from '../piece-sync-service' import { getPiecePackageWithoutArchive, pieceMetadataService } from './piece-metadata-service' @@ -43,6 +46,7 @@ const basePiecesController: FastifyPluginAsyncZod = async (app) => { } const platformId = getPlatformId(req.principal) const projectId = req.query.projectId + await assertProjectAccess({ principal: req.principal, projectId, log: req.log }) const pieceMetadataSummary = await pieceMetadataService(req.log).list({ includeHidden: query.includeHidden ?? false, projectId, @@ -71,6 +75,7 @@ const basePiecesController: FastifyPluginAsyncZod = async (app) => { const decodeScope = decodeURIComponent(scope) const decodedName = decodeURIComponent(name) const platformId = getPlatformId(req.principal) + await assertProjectAccess({ principal: req.principal, projectId: req.query.projectId, log: req.log }) const piece = await pieceMetadataService(req.log).getOrThrow({ platformId, name: `${decodeScope}/${decodedName}`, @@ -91,6 +96,7 @@ const basePiecesController: FastifyPluginAsyncZod = async (app) => { const { version } = req.query const decodedName = decodeURIComponent(name) const platformId = getPlatformId(req.principal) + await assertProjectAccess({ principal: req.principal, projectId: req.query.projectId, log: req.log }) const piece = await pieceMetadataService(req.log).getOrThrow({ platformId, name: decodedName, @@ -151,6 +157,20 @@ const basePiecesController: FastifyPluginAsyncZod = async (app) => { } +async function assertProjectAccess({ principal, projectId, log }: AssertProjectAccessParams): Promise { + if (![ApEdition.ENTERPRISE, ApEdition.CLOUD].includes(system.getEdition())) { + return + } + if (isNil(projectId) || isEmpty(projectId) || isNil(getPlatformId(principal))) { + return + } + await rbacService(log).assertPrinicpalAccessToProject({ + principal, + permission: undefined, + projectId, + }) +} + function getPlatformId(principal: Principal): string | undefined { return principal.type === PrincipalType.WORKER || principal.type === PrincipalType.UNKNOWN || principal.type === PrincipalType.ONBOARDING ? undefined : principal.platform?.id } @@ -251,3 +271,9 @@ const DeletePieceRequest = { }), }, } + +type AssertProjectAccessParams = { + principal: Principal + projectId: string | undefined + log: FastifyBaseLogger +} diff --git a/packages/server/api/test/integration/ce/pieces/piece-metadata.test.ts b/packages/server/api/test/integration/ce/pieces/piece-metadata.test.ts index 3f221f8c7cbb..95677ccd79c2 100644 --- a/packages/server/api/test/integration/ce/pieces/piece-metadata.test.ts +++ b/packages/server/api/test/integration/ce/pieces/piece-metadata.test.ts @@ -12,6 +12,7 @@ import { createMockFlow, createMockFlowVersion, createMockPieceMetadata, + createMockProject, } from '../../../helpers/mocks' import { createMemberContext, createTestContext } from '../../../helpers/test-context' import { setupTestEnvironment, teardownTestEnvironment } from '../../../helpers/test-setup' @@ -192,6 +193,19 @@ describe('Piece Metadata CE API', () => { }) }) + describe('project scoping', () => { + it('does not scope the projectId query param, since piece sets are inert on CE', async () => { + const ctx = await createTestContext(app!) + const member = await createMemberContext(app!, ctx, { projectRole: DefaultProjectRole.VIEWER }) + const otherProject = createMockProject({ ownerId: ctx.user.id, platformId: ctx.platform.id }) + await db.save('project', otherProject) + + const response = await member.get(`/v1/pieces?projectId=${otherProject.id}`) + + expect(response?.statusCode).toBe(StatusCodes.OK) + }) + }) + describe('POST /v1/pieces/sync', () => { it('should sync pieces as platform admin', async () => { const ctx = await createTestContext(app!) diff --git a/packages/server/api/test/integration/ee/pieces/piece-component-filtering.test.ts b/packages/server/api/test/integration/ee/pieces/piece-component-filtering.test.ts index 11872de950f4..96f4a5fb9a12 100644 --- a/packages/server/api/test/integration/ee/pieces/piece-component-filtering.test.ts +++ b/packages/server/api/test/integration/ee/pieces/piece-component-filtering.test.ts @@ -1,12 +1,12 @@ -import { apId } from '@activepieces/core-utils' -import { PackageType, PieceSelectionMode, PieceType, PrincipalType, SuggestionType, TriggerStrategy, TriggerTestStrategy } from '@activepieces/shared' +import { apId, ProjectRole } from '@activepieces/core-utils' +import { DefaultProjectRole, PackageType, PieceSelectionMode, PieceType, PlatformRole, PrincipalType, SuggestionType, TriggerStrategy, TriggerTestStrategy } from '@activepieces/shared' import { FastifyBaseLogger, FastifyInstance } from 'fastify' import { databaseConnection } from '../../../../src/app/database/database-connection' import { pieceCache } from '../../../../src/app/pieces/metadata/piece-cache' import { pieceMetadataService } from '../../../../src/app/pieces/metadata/piece-metadata-service' import { generateMockToken } from '../../../helpers/auth' import { db } from '../../../helpers/db' -import { createMockPieceMetadata, mockAndSaveBasicSetup } from '../../../helpers/mocks' +import { createMockPieceMetadata, createMockProject, createMockProjectMember, mockAndSaveBasicSetup, mockBasicUser } from '../../../helpers/mocks' import { setupTestEnvironment, teardownTestEnvironment } from '../../../helpers/test-setup' let app: FastifyInstance | null = null @@ -585,4 +585,66 @@ describe('Piece Component Filtering (EE)', () => { expect(result).toBeUndefined() }) }) + + describe('project scoping', () => { + async function setupMemberOfOneProject() { + const { mockPlatform, mockProject, mockOwner } = await mockAndSaveBasicSetup({ + plan: { managePiecesEnabled: true }, + }) + + const otherProject = createMockProject({ ownerId: mockOwner.id, platformId: mockPlatform.id }) + await db.save('project', otherProject) + + const { mockUser } = await mockBasicUser({ + user: { platformId: mockPlatform.id, platformRole: PlatformRole.MEMBER }, + }) + const projectRole = await db.findOneByOrFail('project_role', { name: DefaultProjectRole.ADMIN }) + await db.save('project_member', createMockProjectMember({ + userId: mockUser.id, + platformId: mockPlatform.id, + projectId: mockProject.id, + projectRoleId: projectRole.id, + })) + + const token = await generateMockToken({ + type: PrincipalType.USER, + id: mockUser.id, + platform: { id: mockPlatform.id }, + }) + + return { token, mockProject, otherProject } + } + + it('a member of the project can list its pieces', async () => { + const { token, mockProject } = await setupMemberOfOneProject() + + const response = await app!.inject({ method: 'GET', url: `/api/v1/pieces?projectId=${mockProject.id}`, headers: { authorization: `Bearer ${token}` } }) + + expect(response.statusCode).toBe(200) + }) + + it('an empty projectId is no project at all', async () => { + const { token } = await setupMemberOfOneProject() + + const response = await app!.inject({ method: 'GET', url: '/api/v1/pieces?projectId=', headers: { authorization: `Bearer ${token}` } }) + + expect(response.statusCode).toBe(200) + }) + + it('a non-member cannot list another project pieces', async () => { + const { token, otherProject } = await setupMemberOfOneProject() + + const response = await app!.inject({ method: 'GET', url: `/api/v1/pieces?projectId=${otherProject.id}`, headers: { authorization: `Bearer ${token}` } }) + + expect(response.statusCode).toBe(403) + }) + + it('a non-member cannot fetch a single piece scoped to another project', async () => { + const { token, otherProject } = await setupMemberOfOneProject() + + const response = await app!.inject({ method: 'GET', url: `/api/v1/pieces/test-piece?projectId=${otherProject.id}`, headers: { authorization: `Bearer ${token}` } }) + + expect(response.statusCode).toBe(403) + }) + }) }) diff --git a/packages/web/AGENTS.md b/packages/web/AGENTS.md index 2e4807829d18..4a92609659ee 100644 --- a/packages/web/AGENTS.md +++ b/packages/web/AGENTS.md @@ -32,6 +32,7 @@ You are working in the Activepieces web application (`packages/web`). ## Tailwind / Styling - **Always use `cn()` from `@/lib/utils` for className composition.** It uses `clsx` + `tailwind-merge` and handles conflicts and conditionals correctly. Never use template literals (`` `class-a ${someVar}` ``) or string concatenation for `className` props. +- **Use the predefined type scale, never an arbitrary font size.** This is Tailwind v4 and the theme lives in the `@theme` block of `src/styles.css` — it is *not* stock Tailwind: it adds `--text-xss` (10.4px) and shrinks `--text-3xl` to 1.75rem and `--text-4xl` to 2rem. Pick the token, never `text-[13px]`: 10-11px → `text-xss` (eyebrows, dense badges), 11.5-12.5px → `text-xs` (metadata), 13-13.5px → `text-sm` (**body default**), 15-15.5px → `text-base`, then `text-lg` (card titles), `text-xl` (section titles), `text-2xl` (page titles), `text-3xl` / `text-4xl` (display). Drop the class entirely when the component already sets it (a `Badge` is `text-xs` on its own). Same for arbitrary `leading-[...]` / `tracking-[...]`: use `leading-*`, `tracking-tight` for headings, `tracking-wide` / `tracking-wider` for uppercase eyebrows. Fractional spacing is valid in v4, so `size-4.5` beats `size-[18px]`. The one exception is a layout constraint with no token equivalent (`max-w-[628px]` for a reading measure, `lg:w-[344px]` for a sidebar) — those stay arbitrary and are idiomatic. Neither eslint nor `tsc` catches any of this, so it only ever surfaces in review. - **Never use negative margins** (`-mt-`, `-mb-`, `-mx-`, `-my-`, `-ml-`, `-mr-`, etc.). They introduce subtle layout bugs and make spacing hard to reason about. Use `gap`, `padding`, or `space-*` utilities instead. ## Components diff --git a/packages/web/public/locales/en/translation.json b/packages/web/public/locales/en/translation.json index a02ce779091f..22e23aaea94b 100644 --- a/packages/web/public/locales/en/translation.json +++ b/packages/web/public/locales/en/translation.json @@ -2655,6 +2655,25 @@ "revokeSelectedCount": "{count, plural, =1 {Revoke 1} other {Revoke #}}", "revokedGrants": "{count, plural, =1 {1 connection} other {# connections}}", "{name} · you": "{name} · you", + "Every piece a connected client can reach, and every action inside it.": "Every piece a connected client can reach, and every action inside it.", + "This page is a mirror — a platform admin decides what is on the list.": "This page is a mirror — a platform admin decides what is on the list.", + "Nothing below can run right now": "Nothing below can run right now", + "Running piece actions is switched off for this project. Clients can still see the list, but every call fails.": "Running piece actions is switched off for this project. Clients can still see the list, but every call fails.", + "Turn it on in project settings": "Turn it on in project settings", + "Search pieces and actions...": "Search pieces and actions...", + "No piece or action matches your search.": "No piece or action matches your search.", + "No pieces are reachable in this project.": "No pieces are reachable in this project.", + "You cannot see this project": "You cannot see this project", + "Pick another project above, or ask a platform admin for access to this one.": "Pick another project above, or ask a platform admin for access to this one.", + "The pieces failed to load": "The pieces failed to load", + "Nothing is listed below because the request failed, not because the project is empty.": "Nothing is listed below because the request failed, not because the project is empty.", + "Show {count} more pieces": "Show {count} more pieces", + "Every piece below is reachable by any connected client. Restricting the list to a chosen set is an enterprise feature.": "Every piece below is reachable by any connected client. Restricting the list to a chosen set is an enterprise feature.", + "This project's pieces are controlled by a Piece Set.": "This project's pieces are controlled by a Piece Set.", + "Review piece set": "Review piece set", + "Can delete or overwrite data in {pieceName}.": "Can delete or overwrite data in {pieceName}.", + "pieceDestructiveActionCount": "{count, plural, =1 {1 destructive} other {# destructive}}", + "pieceActionCount": "{count, plural, =1 {1 action} other {# actions}}", "Opens Cursor and writes the server into ~/.cursor/mcp.json.": "Opens Cursor and writes the server into ~/.cursor/mcp.json.", "Paste the server URL as a custom connector. This client dials your server from the internet, so localhost will not reach it.": "Paste the server URL as a custom connector. This client dials your server from the internet, so localhost will not reach it.", "Paste the server URL as a connector. ChatGPT dials your server from the internet, so localhost will not reach it.": "Paste the server URL as a connector. ChatGPT dials your server from the internet, so localhost will not reach it.", diff --git a/packages/web/src/app/routes/mcp-server/index.tsx b/packages/web/src/app/routes/mcp-server/index.tsx index 95617dd5c828..c3d8455f32f0 100644 --- a/packages/web/src/app/routes/mcp-server/index.tsx +++ b/packages/web/src/app/routes/mcp-server/index.tsx @@ -9,6 +9,7 @@ import { GrantsTab } from './grants/grants-tab'; import { useMcpNav } from './mcp-nav'; import { useMcpServerUrl } from './mcp-server-url'; import { PageBand } from './page-band'; +import { PiecesTab } from './pieces/pieces-tab'; export default function McpServerPage() { const { serverUrl, isReachableFromInternet } = useMcpServerUrl(); @@ -25,6 +26,9 @@ export default function McpServerPage() { {t('Connect')} + + {t('Pieces')} + {t('Connections')} @@ -33,7 +37,12 @@ export default function McpServerPage() {
- {nav.tab === 'connections' ? ( + {nav.tab === 'pieces' ? ( + + ) : nav.tab === 'connections' ? ( ) : ( setParams({}), showBrowse: () => setParams({ browse: '1' }), showClient: (key: string) => setParams({ client: key }), showTab: (value: string) => navigate(`/mcp-server/${toTab(value)}`), + selectProject: (projectId: string) => setParams({ project: projectId }), }; } -export type McpTab = 'connect' | 'connections'; +export type McpTab = 'connect' | 'pieces' | 'connections'; export type McpView = 'landing' | 'browse' | 'client'; @@ -29,8 +33,10 @@ export type McpNav = { tab: McpTab; view: McpView; clientKey: string | null; + projectId: string | null; showLanding: () => void; showBrowse: () => void; showClient: (key: string) => void; showTab: (value: string) => void; + selectProject: (projectId: string) => void; }; diff --git a/packages/web/src/app/routes/mcp-server/pieces/piece-row.tsx b/packages/web/src/app/routes/mcp-server/pieces/piece-row.tsx new file mode 100644 index 000000000000..e979559d46d0 --- /dev/null +++ b/packages/web/src/app/routes/mcp-server/pieces/piece-row.tsx @@ -0,0 +1,158 @@ +import type { ActionClassification } from '@activepieces/pieces-framework'; +import { t } from 'i18next'; +import { ChevronDown } from 'lucide-react'; +import { memo, useState } from 'react'; + +import { TextWithTooltip } from '@/components/custom/text-with-tooltip'; +import { Badge } from '@/components/ui/badge'; +import { + Collapsible, + CollapsibleContent, + CollapsibleTrigger, +} from '@/components/ui/collapsible'; +import { PieceIcon } from '@/features/pieces'; +import { ACTION_CLASSIFICATION_BADGES } from '@/features/pieces/utils/action-classification'; +import { cn } from '@/lib/utils'; + +import { ActionGroup, ReachablePiece } from './pieces-utils'; + +export const PieceRow = memo(function PieceRow({ + row, + isLastRow, +}: PieceRowProps) { + const [isOpenedByUser, setIsOpenedByUser] = useState(false); + const isOpen = row.forceExpanded || isOpenedByUser; + + return ( + + + +
+ +
{row.piece.displayName}
+
+ +
+ {row.piece.description} +
+
+
+
+ {row.destructiveActionCount > 0 && ( + + {t('pieceDestructiveActionCount', { + count: row.destructiveActionCount, + })} + + )} + + {t('pieceActionCount', { + count: row.actionCount, + })} + + +
+
+ +
+ {row.groups.map((group) => ( + + ))} +
+
+
+ ); +}); + +function ActionGroupColumn({ + group, + pieceDisplayName, +}: ActionGroupColumnProps) { + const tone = CLASSIFICATION_TONES[group.classification]; + + return ( +
+
+ + {ACTION_CLASSIFICATION_BADGES[group.classification].label()} + + + {group.actions.length} + +
+
+ {group.actions.map((action) => ( + +
+ {action.displayName} +
+
+ ))} + {group.classification === 'DESTRUCTIVE' && ( +

+ {t('Can delete or overwrite data in {pieceName}.', { + pieceName: pieceDisplayName, + })} +

+ )} +
+
+ ); +} + +const CLASSIFICATION_TONES: Record = { + READ: { label: 'text-foreground', count: 'accent' }, + SEARCH: { label: 'text-foreground', count: 'accent' }, + WRITE: { + label: 'text-warning-700 dark:text-warning-300', + count: 'warning', + }, + DESTRUCTIVE: { + label: 'text-destructive-700 dark:text-destructive-300', + count: 'destructive', + frame: + 'gap-0.5 rounded-md border border-destructive-200 bg-destructive-50 py-1.5 dark:border-destructive-900 dark:bg-destructive-950/30', + }, +}; + +type ClassificationTone = { + label: string; + count: 'accent' | 'warning' | 'destructive'; + frame?: string; +}; + +type ActionGroupColumnProps = { + group: ActionGroup; + pieceDisplayName: string; +}; + +type PieceRowProps = { + row: ReachablePiece; + isLastRow: boolean; +}; diff --git a/packages/web/src/app/routes/mcp-server/pieces/pieces-tab.tsx b/packages/web/src/app/routes/mcp-server/pieces/pieces-tab.tsx new file mode 100644 index 000000000000..60df72c2aa1e --- /dev/null +++ b/packages/web/src/app/routes/mcp-server/pieces/pieces-tab.tsx @@ -0,0 +1,289 @@ +import { ErrorCode } from '@activepieces/core-utils'; +import { isNil, SuggestionType } from '@activepieces/shared'; +import { t } from 'i18next'; +import { ExternalLink, Info, TriangleAlert } from 'lucide-react'; +import { useMemo, useState } from 'react'; +import { Link } from 'react-router-dom'; +import { useDebounce } from 'use-debounce'; + +import { ProjectSettingsDialog } from '@/app/components/project-settings'; +import { mcpHooks } from '@/app/components/project-settings/mcp-server/utils/mcp-hooks'; +import { RequestTrial } from '@/app/components/request-trial'; +import { LockedAlert } from '@/components/custom/locked-alert'; +import { SearchInput } from '@/components/custom/search-input'; +import { Alert, AlertDescription, AlertTitle } from '@/components/ui/alert'; +import { Button } from '@/components/ui/button'; +import { Skeleton } from '@/components/ui/skeleton'; +import { VirtualizedList } from '@/components/ui/virtualized-list'; +import { pieceSetQueries } from '@/features/piece-sets'; +import { piecesHooks } from '@/features/pieces/hooks/pieces-hooks'; +import { projectCollectionUtils } from '@/features/projects'; +import { useIsPlatformAdmin } from '@/hooks/authorization-hooks'; +import { platformHooks } from '@/hooks/platform-hooks'; +import { api } from '@/lib/api'; +import { authenticationSession } from '@/lib/authentication-session'; + +import { PageBand } from '../page-band'; + +import { PieceRow } from './piece-row'; +import { piecesUtils } from './pieces-utils'; +import { ProjectPicker } from './project-picker'; + +const RUN_ACTION_TOOL_NAME = 'ap_run_action'; +const COLLAPSED_ROW_LIMIT = 6; +const COLLAPSED_ROW_HEIGHT = 50; +const PIECE_SETS_LIST_ROUTE = '/platform/setup/pieces?tab=piece-sets'; +const SEARCH_DEBOUNCE_MS = 300; + +export function PiecesTab({ projectId, onSelectProject }: PiecesTabProps) { + const [searchQuery, setSearchQuery] = useState(''); + const [debouncedSearchQuery] = useDebounce( + searchQuery.trim(), + SEARCH_DEBOUNCE_MS, + ); + const [showAll, setShowAll] = useState(false); + + const isSearching = debouncedSearchQuery !== ''; + const { pieces, isLoading, isError, error, refetch } = piecesHooks.usePieces({ + projectId: projectId ?? undefined, + searchQuery: isSearching ? debouncedSearchQuery : undefined, + suggestionType: SuggestionType.ACTION, + enabled: !isNil(projectId), + keepPreviousResults: true, + }); + const { data: mcpServer } = mcpHooks.useMcpServer(projectId ?? ''); + + const rows = useMemo( + () => + piecesUtils.toReachablePieces({ + pieces: pieces ?? [], + isSearching, + }), + [pieces, isSearching], + ); + const visibleRows = useMemo( + () => (isSearching || showAll ? rows : rows.slice(0, COLLAPSED_ROW_LIMIT)), + [rows, isSearching, showAll], + ); + const hiddenCount = rows.length - visibleRows.length; + + return ( + +
+

+ {t( + 'Every piece a connected client can reach, and every action inside it.', + )} +

+

+ {t( + 'This page is a mirror — a platform admin decides what is on the list.', + )} +

+
+ + + + {projectId !== null && + mcpServer?.disabledTools?.includes(RUN_ACTION_TOOL_NAME) && ( + + )} + +
+ +
+ +
+
+ + {isLoading ? ( +
+ {Array.from({ length: COLLAPSED_ROW_LIMIT }).map((_, index) => ( + + ))} +
+ ) : isError ? ( + + ) : rows.length === 0 ? ( +
+ {isSearching + ? t('No piece or action matches your search.') + : t('No pieces are reachable in this project.')} +
+ ) : ( +
+ visibleRows[index].piece.name} + renderItem={(row, index) => ( + + )} + /> + {hiddenCount > 0 && ( + + )} +
+ )} +
+ ); +} + +function PiecesUnavailableAlert({ + error, + onRetry, +}: PiecesUnavailableAlertProps) { + if (isProjectAccessError(error)) { + return ( + + + {t('You cannot see this project')} + + {t( + 'Pick another project above, or ask a platform admin for access to this one.', + )} + + + ); + } + + return ( + + + {t('The pieces failed to load')} + + {t( + 'Nothing is listed below because the request failed, not because the project is empty.', + )} + + + + ); +} + +function RunActionDisabledAlert({ projectId }: { projectId: string }) { + const [settingsOpen, setSettingsOpen] = useState(false); + const isCurrentProject = authenticationSession.getProjectId() === projectId; + + return ( + <> + + + {t('Nothing below can run right now')} + + {t( + 'Running piece actions is switched off for this project. Clients can still see the list, but every call fails.', + )} + + {isCurrentProject && ( + + )} + + setSettingsOpen(false)} + initialTab="mcp" + /> + + ); +} + +function PieceSetBanner({ projectId }: { projectId: string | null }) { + const { platform } = platformHooks.useCurrentPlatform(); + const isPlatformAdmin = useIsPlatformAdmin(); + const { data: projects = [] } = projectCollectionUtils.useAll(); + const pieceSetId = + projects.find((project) => project.id === projectId)?.pieceSetId ?? null; + const { data: pieceSet } = pieceSetQueries.usePieceSet(pieceSetId ?? ''); + + if (!platform.plan.managePiecesEnabled) { + return ( + + } + /> + ); + } + + return ( + + + + {isPlatformAdmin + ? t("This project's pieces are controlled by a Piece Set.") + : t( + "This project's pieces are controlled by a Piece Set. Contact a platform admin to change it.", + )} + + {isPlatformAdmin && ( + + )} + + ); +} + +function isProjectAccessError(error: Error | null): boolean { + return ( + api.isApError(error, ErrorCode.AUTHORIZATION) || + api.isApError(error, ErrorCode.PERMISSION_DENIED) || + api.isApError(error, ErrorCode.ENTITY_NOT_FOUND) + ); +} + +type PiecesUnavailableAlertProps = { + error: Error | null; + onRetry: () => void; +}; + +type PiecesTabProps = { + projectId: string | null; + onSelectProject: (projectId: string) => void; +}; diff --git a/packages/web/src/app/routes/mcp-server/pieces/pieces-utils.ts b/packages/web/src/app/routes/mcp-server/pieces/pieces-utils.ts new file mode 100644 index 000000000000..d8af6487c389 --- /dev/null +++ b/packages/web/src/app/routes/mcp-server/pieces/pieces-utils.ts @@ -0,0 +1,89 @@ +import type { + ActionBase, + ActionClassification, + PieceMetadataModelSummary, +} from '@activepieces/pieces-framework'; + +import { pieceSearchUtils } from '@/features/pieces/utils/piece-search-utils'; + +const CLASSIFICATION_ORDER: ActionClassification[] = [ + 'READ', + 'SEARCH', + 'WRITE', + 'DESTRUCTIVE', +]; + +const DEFAULT_CLASSIFICATION: ActionClassification = 'WRITE'; + +function groupByClassification(actions: ActionBase[]): ActionGroup[] { + return CLASSIFICATION_ORDER.map((classification) => ({ + classification, + actions: actions.filter( + (action) => + (action.classification ?? DEFAULT_CLASSIFICATION) === classification, + ), + })).filter((group) => group.actions.length > 0); +} + +function orderPopularFirst( + pieces: PieceMetadataModelSummary[], +): PieceMetadataModelSummary[] { + const popularPieceNames = pieceSearchUtils.POPULAR_PIECES_NAMES; + const rank = (piece: PieceMetadataModelSummary) => { + const index = popularPieceNames.indexOf(piece.name); + return index === -1 ? popularPieceNames.length : index; + }; + return [...pieces].sort( + (a, b) => rank(a) - rank(b) || a.displayName.localeCompare(b.displayName), + ); +} + +function toReachablePiece({ + piece, + isSearching, +}: { + piece: PieceMetadataModelSummary; + isSearching: boolean; +}): ReachablePiece { + const actions = piece.suggestedActions ?? []; + return { + piece, + groups: groupByClassification(actions), + actionCount: actions.length, + destructiveActionCount: actions.filter( + (action) => action.classification === 'DESTRUCTIVE', + ).length, + forceExpanded: isSearching, + }; +} + +function toReachablePieces({ + pieces, + isSearching, +}: { + pieces: PieceMetadataModelSummary[]; + isSearching: boolean; +}): ReachablePiece[] { + const piecesWithActions = pieces.filter( + (piece) => (piece.suggestedActions ?? []).length > 0, + ); + const orderedPieces = isSearching + ? piecesWithActions + : orderPopularFirst(piecesWithActions); + return orderedPieces.map((piece) => toReachablePiece({ piece, isSearching })); +} + +export const piecesUtils = { toReachablePieces }; + +export type ActionGroup = { + classification: ActionClassification; + actions: ActionBase[]; +}; + +export type ReachablePiece = { + piece: PieceMetadataModelSummary; + groups: ActionGroup[]; + actionCount: number; + destructiveActionCount: number; + forceExpanded: boolean; +}; diff --git a/packages/web/src/app/routes/mcp-server/pieces/project-picker.tsx b/packages/web/src/app/routes/mcp-server/pieces/project-picker.tsx new file mode 100644 index 000000000000..02055d3ea589 --- /dev/null +++ b/packages/web/src/app/routes/mcp-server/pieces/project-picker.tsx @@ -0,0 +1,71 @@ +import { t } from 'i18next'; +import { Check, ChevronDown } from 'lucide-react'; + +import { Button } from '@/components/ui/button'; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from '@/components/ui/dropdown-menu'; +import { + ApProjectDisplay, + getProjectName, + projectCollectionUtils, +} from '@/features/projects'; +import { cn } from '@/lib/utils'; + +type ProjectPickerProps = { + projectId: string | null; + onSelect: (projectId: string) => void; +}; + +export function ProjectPicker({ projectId, onSelect }: ProjectPickerProps) { + const { data: projects = [] } = projectCollectionUtils.useAll(); + const selectedProject = projects.find((project) => project.id === projectId); + + return ( + + + + + + {projects.map((project) => ( + onSelect(project.id)} + > + + + + ))} + + + ); +} diff --git a/packages/web/src/features/pieces/hooks/pieces-hooks.ts b/packages/web/src/features/pieces/hooks/pieces-hooks.ts index 07a3ae5bed63..eba10851e176 100644 --- a/packages/web/src/features/pieces/hooks/pieces-hooks.ts +++ b/packages/web/src/features/pieces/hooks/pieces-hooks.ts @@ -15,10 +15,12 @@ import { FlowTriggerType, ApFlagId, ApEnvironment, + SuggestionType, TelemetryEventName, } from '@activepieces/shared'; import { QueryClient, + QueryKey, useMutation, usePrefetchQuery, useQueries, @@ -78,10 +80,15 @@ type UseMultiplePiecesProps = { }; type UsePiecesProps = { + projectId?: string; searchQuery?: string; includeHidden?: boolean; isTableQuery?: boolean; skipProjectFilter?: boolean; + suggestionType?: SuggestionType; + enabled?: boolean; + keepPreviousResults?: boolean; + showErrorDialog?: boolean; }; type UsePrefetchPiecesProps = { skipProjectFilter?: boolean; @@ -179,27 +186,39 @@ export const piecesHooks = { return { summary, isLoading }; }, usePieces: ({ + projectId, searchQuery, includeHidden = false, isTableQuery = false, skipProjectFilter = false, + suggestionType, + enabled = true, + keepPreviousResults = false, + showErrorDialog, }: UsePiecesProps) => { const { i18n } = useTranslation(); const query = useQuery({ ...piecesQueryOptions({ + projectId, searchQuery, includeHidden, isTableQuery, skipProjectFilter, + suggestionType, locale: i18n.language as LocalesEnum, + keepPreviousResults, }), - meta: isTableQuery - ? { showErrorDialog: true, loadSubsetOptions: {} } - : undefined, + enabled, + meta: + showErrorDialog ?? isTableQuery + ? { showErrorDialog: true, loadSubsetOptions: {} } + : undefined, }); return { pieces: query.data, isLoading: query.isLoading, + isError: query.isError, + error: query.error, refetch: query.refetch, }; }, @@ -585,32 +604,59 @@ function invalidatePieceCaches(queryClient: QueryClient): Promise { export const pieceCacheUtils = { invalidatePieceCaches }; function piecesQueryOptions({ + projectId, searchQuery, includeHidden, isTableQuery, skipProjectFilter, + suggestionType, locale, + keepPreviousResults = false, }: { + projectId?: string; searchQuery?: string; includeHidden: boolean; isTableQuery: boolean; skipProjectFilter: boolean; + suggestionType?: SuggestionType; locale: LocalesEnum; + keepPreviousResults?: boolean; }) { - const projectId = skipProjectFilter + const queriedProjectId = skipProjectFilter ? undefined - : authenticationSession.getProjectId()!; + : projectId ?? authenticationSession.getProjectId() ?? undefined; return { queryKey: [ isTableQuery ? 'pieces-table' : 'pieces', + queriedProjectId, searchQuery, includeHidden, skipProjectFilter, - projectId, + suggestionType, locale, ], queryFn: () => - piecesApi.list({ projectId, searchQuery, includeHidden, locale }), - staleTime: searchQuery ? 0 : Infinity, + piecesApi.list({ + projectId: queriedProjectId, + searchQuery, + includeHidden, + suggestionType, + locale, + }), + staleTime: searchQuery ? SEARCH_RESULTS_STALE_TIME_MS : Infinity, + ...(keepPreviousResults + ? { + placeholderData: ( + previousPieces: PieceMetadataModelSummary[] | undefined, + previousQuery: { queryKey: QueryKey } | undefined, + ) => + previousQuery?.queryKey[PROJECT_ID_KEY_INDEX] === queriedProjectId + ? previousPieces + : undefined, + } + : {}), }; } + +const SEARCH_RESULTS_STALE_TIME_MS = 5 * 60 * 1000; +const PROJECT_ID_KEY_INDEX = 1; diff --git a/packages/web/src/features/pieces/hooks/steps-hooks.ts b/packages/web/src/features/pieces/hooks/steps-hooks.ts index ccdf5b1b0e31..043035857ee0 100644 --- a/packages/web/src/features/pieces/hooks/steps-hooks.ts +++ b/packages/web/src/features/pieces/hooks/steps-hooks.ts @@ -53,7 +53,7 @@ export const stepsHooks = { }, useAllStepsMetadata: ({ searchQuery, type, enabled }: UseMetadataProps) => { const { i18n } = useTranslation(); - const projectId = authenticationSession.getProjectId()!; + const projectId = authenticationSession.getProjectId() ?? undefined; const query = useQuery({ queryKey: [ 'pieces-metadata', diff --git a/packages/web/src/lib/api.ts b/packages/web/src/lib/api.ts index 0768fbdbb5e3..7bde9523324b 100644 --- a/packages/web/src/lib/api.ts +++ b/packages/web/src/lib/api.ts @@ -170,8 +170,8 @@ export const api = { if (!isAxiosError(error)) { return false; } - const responseData = error.response?.data as ApErrorParams; - return responseData.code === errorCode; + const responseData = error.response?.data as ApErrorParams | undefined; + return responseData?.code === errorCode; }, isError(error: unknown): error is HttpError { return isAxiosError(error); diff --git a/packages/web/test/app/routes/mcp-server/pieces/pieces-utils.test.ts b/packages/web/test/app/routes/mcp-server/pieces/pieces-utils.test.ts new file mode 100644 index 000000000000..0a1d7d53f85c --- /dev/null +++ b/packages/web/test/app/routes/mcp-server/pieces/pieces-utils.test.ts @@ -0,0 +1,167 @@ +import type { + ActionBase, + PieceMetadataModelSummary, +} from '@activepieces/pieces-framework'; +import { describe, expect, it } from 'vitest'; + +import { piecesUtils } from '@/app/routes/mcp-server/pieces/pieces-utils'; + +function action( + displayName: string, + classification?: ActionBase['classification'] +): ActionBase { + return { + name: displayName.toLowerCase().replace(/\s/g, '_'), + displayName, + description: `${displayName} description`, + props: {}, + requireAuth: true, + classification, + }; +} + +function piece( + overrides: Partial & + Pick +): PieceMetadataModelSummary { + return { + description: '', + actions: 0, + triggers: 0, + suggestedActions: [], + ...overrides, + } as PieceMetadataModelSummary; +} + +const slack = piece({ + name: '@activepieces/piece-slack', + displayName: 'Slack', + description: 'Send messages, read channels', + suggestedActions: [ + action('Get User', 'READ'), + action('List Users', 'SEARCH'), + action('Send Message', 'WRITE'), + action('Archive Channel', 'DESTRUCTIVE'), + ], +}); + +const gmail = piece({ + name: '@activepieces/piece-gmail', + displayName: 'Gmail', + description: 'Read the inbox', + suggestedActions: [action('Send Email', 'WRITE')], +}); + +describe('piecesUtils.toReachablePieces', () => { + it('returns every piece collapsed when there is no query', () => { + const rows = piecesUtils.toReachablePieces({ + pieces: [slack, gmail], + isSearching: false, + }); + + expect(rows).toHaveLength(2); + expect(rows.every((row) => row.forceExpanded)).toBe(false); + expect(rows[0].actionCount).toBe(4); + }); + + it('counts destructive actions per piece', () => { + const [slackRow, gmailRow] = piecesUtils.toReachablePieces({ + pieces: [slack, gmail], + isSearching: false, + }); + + expect(slackRow.destructiveActionCount).toBe(1); + expect(gmailRow.destructiveActionCount).toBe(0); + }); + + it('groups actions in READ, SEARCH, WRITE, DESTRUCTIVE order and omits empty groups', () => { + const [slackRow, gmailRow] = piecesUtils.toReachablePieces({ + pieces: [slack, gmail], + isSearching: false, + }); + + expect(slackRow.groups.map((group) => group.classification)).toEqual([ + 'READ', + 'SEARCH', + 'WRITE', + 'DESTRUCTIVE', + ]); + expect(gmailRow.groups.map((group) => group.classification)).toEqual([ + 'WRITE', + ]); + }); + + it('treats an unclassified action as WRITE, never as read-only', () => { + const unknown = piece({ + name: '@activepieces/piece-unknown', + displayName: 'Unknown', + suggestedActions: [action('Do Something', undefined)], + }); + + const [row] = piecesUtils.toReachablePieces({ + pieces: [unknown], + isSearching: false, + }); + + expect(row.groups).toEqual([ + expect.objectContaining({ classification: 'WRITE' }), + ]); + expect(row.destructiveActionCount).toBe(0); + }); + + it('keeps the server relevance order and expands every row while searching', () => { + const rows = piecesUtils.toReachablePieces({ + pieces: [gmail, slack], + isSearching: true, + }); + + expect(rows.map((row) => row.piece.displayName)).toEqual([ + 'Gmail', + 'Slack', + ]); + expect(rows.every((row) => row.forceExpanded)).toBe(true); + }); + + it('orders popular pieces first only when not searching', () => { + const rows = piecesUtils.toReachablePieces({ + pieces: [gmail, slack], + isSearching: false, + }); + + expect(rows.map((row) => row.piece.displayName)).toEqual([ + 'Slack', + 'Gmail', + ]); + }); + + it('reports what the server returned for a piece, not its whole catalogue', () => { + const narrowedSlack = piece({ + name: '@activepieces/piece-slack', + displayName: 'Slack', + suggestedActions: [action('Archive Channel', 'DESTRUCTIVE')], + }); + + const [row] = piecesUtils.toReachablePieces({ + pieces: [narrowedSlack], + isSearching: true, + }); + + expect(row.actionCount).toBe(1); + expect(row.destructiveActionCount).toBe(1); + expect(row.groups).toEqual([ + expect.objectContaining({ classification: 'DESTRUCTIVE' }), + ]); + }); + + it('drops pieces that expose no actions at all', () => { + const actionless = piece({ + name: '@activepieces/piece-actionless', + displayName: 'Actionless', + suggestedActions: [], + }); + + expect( + piecesUtils.toReachablePieces({ pieces: [actionless], isSearching: false }) + ).toEqual([]); + }); +}); diff --git a/packages/web/test/features/pieces/pieces-hooks.test.tsx b/packages/web/test/features/pieces/pieces-hooks.test.tsx new file mode 100644 index 000000000000..0ca4d4da71be --- /dev/null +++ b/packages/web/test/features/pieces/pieces-hooks.test.tsx @@ -0,0 +1,116 @@ +// @vitest-environment jsdom +import { PieceMetadataModelSummary } from '@activepieces/pieces-framework'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, waitFor } from '@testing-library/react'; +import { ReactNode } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('i18next', () => ({ t: (key: string) => key })); +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ i18n: { language: 'en' } }), +})); +vi.mock('@/components/providers/telemetry-provider', () => ({ + useTelemetry: () => ({ capture: vi.fn() }), +})); +vi.mock('@/hooks/flags-hooks', () => ({ + flagsHooks: { useFlag: () => ({ data: undefined }) }, +})); +vi.mock('@/hooks/platform-hooks', () => ({ + platformHooks: { useCurrentPlatform: () => ({ platform: { plan: {} } }) }, +})); +vi.mock('@/lib/authentication-session', () => ({ + authenticationSession: { getProjectId: () => 'fallback_project' }, +})); +vi.mock('@/features/pieces/stores/piece-selector-tabs-provider', () => ({ + PieceSelectorTabType: {}, + usePieceSelectorTabs: () => ({ + selectedTab: undefined, + selectedCustomTabId: undefined, + }), +})); + +const list = vi.fn(); +vi.mock('@/features/pieces/api/pieces-api', () => ({ + piecesApi: { + list: (request: { projectId?: string; searchQuery?: string }) => + list(request), + }, +})); + +import { piecesHooks } from '@/features/pieces/hooks/pieces-hooks'; + +describe('usePieces with keepPreviousResults', () => { + let queryClient: QueryClient; + + beforeEach(() => { + list.mockReset(); + queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + }); + + afterEach(() => { + queryClient.clear(); + }); + + it('drops the previous project rows while the new project is pending', async () => { + list.mockImplementation(({ projectId }: { projectId?: string }) => + projectId === PROJECT_A + ? Promise.resolve([pieceNamed('slack')]) + : neverResolves(), + ); + + const { result, rerender } = renderPieces({ projectId: PROJECT_A }); + await waitFor(() => expect(result.current.pieces).toHaveLength(1)); + + rerender({ projectId: PROJECT_B }); + + expect(result.current.pieces).toBeUndefined(); + }); + + it('keeps the rows while only the search term changes', async () => { + list.mockImplementation(({ searchQuery }: { searchQuery?: string }) => + searchQuery === undefined + ? Promise.resolve([pieceNamed('slack')]) + : neverResolves(), + ); + + const { result, rerender } = renderPieces({ projectId: PROJECT_A }); + await waitFor(() => expect(result.current.pieces).toHaveLength(1)); + + rerender({ projectId: PROJECT_A, searchQuery: 'send' }); + + expect(result.current.pieces).toEqual([pieceNamed('slack')]); + }); + + function renderPieces(initialProps: HookProps) { + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + return renderHook( + ({ projectId, searchQuery }: HookProps) => + piecesHooks.usePieces({ + projectId, + searchQuery, + keepPreviousResults: true, + }), + { initialProps, wrapper }, + ); + } +}); + +function pieceNamed(name: string): PieceMetadataModelSummary { + return { name } as PieceMetadataModelSummary; +} + +function neverResolves(): Promise { + return new Promise(() => undefined); +} + +const PROJECT_A = 'project_a'; +const PROJECT_B = 'project_b'; + +type HookProps = { + projectId: string; + searchQuery?: string; +}; From 7db39e2340bdd9987cd28ec1fcd210a99e2ecd8e Mon Sep 17 00:00:00 2001 From: Othman Abu Ajamieh <52608229+othmanemad@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:40:56 +0300 Subject: [PATCH 2/3] fix(webhooks): answer a sync webhook with 500 when the run fails (#15226) --- .../engineering/ci-pr-review-hygiene.md | 3 + brain/knowledge/eventing-webhooks/webhooks.md | 10 +- brain/knowledge/execution-runtime/index.md | 2 + brain/knowledge/flows-execution/flow-runs.md | 1 + bun.lock | 4 +- docs/install/reference/breaking-changes.mdx | 14 ++ docs/install/reference/limits.mdx | 11 +- packages/core/execution/package.json | 2 +- .../core/execution/src/lib/engine/requests.ts | 2 + .../flow-run/engine-run-callback-service.ts | 26 +++- .../flows/flow-run/execute-flow-e2e.test.ts | 145 ++++++++++++++++++ ...-run-callback-failed-sync-response.test.ts | 113 ++++++++++++++ .../lib/helper/flow-run-progress-reporter.ts | 2 + .../src/lib/execute/jobs/execute-flow.ts | 4 +- .../lib/execute/jobs/execute-flow.test.ts | 84 ++++++++++ 15 files changed, 413 insertions(+), 10 deletions(-) create mode 100644 packages/server/api/test/unit/app/flows/flow-run/engine-run-callback-failed-sync-response.test.ts diff --git a/brain/knowledge/engineering/ci-pr-review-hygiene.md b/brain/knowledge/engineering/ci-pr-review-hygiene.md index 481dc56d1e11..d596c5b0488a 100644 --- a/brain/knowledge/engineering/ci-pr-review-hygiene.md +++ b/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 -- ` instead.** `git stash push -- ` 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 -- `, run, then `git checkout HEAD -- `. 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 ` 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/, 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- | 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/.../.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:/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 -- /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/...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 ` 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. diff --git a/brain/knowledge/eventing-webhooks/webhooks.md b/brain/knowledge/eventing-webhooks/webhooks.md index b832b187ebc6..905be3e104aa 100644 --- a/brain/knowledge/eventing-webhooks/webhooks.md +++ b/brain/knowledge/eventing-webhooks/webhooks.md @@ -20,7 +20,7 @@ Webhooks are the primary entry point for event-driven flow execution from outsid - `/:flowId/draft/sync` and `/:flowId/draft` — testing against the draft version. - `/:flowId/test` — captures request as sample data, no execution. - **Async**: offload payload to S3/DB if over `AP_WEBHOOK_PAYLOAD_INLINE_THRESHOLD_KB` (default 512KB) → queue `EXECUTE_WEBHOOK` → return 200. Job carries a `JobPayload` union (`inline` or `ref`); the **engine** resolves it at execution time (workers no longer fetch payloads). -- **Sync**: create FlowRun with `WEBHOOK_RESPONSE` → register `engineResponseWatcher` → wait (`AP_WEBHOOK_TIMEOUT_SECONDS`, default 30; callers can override, e.g. MCP uses 5 min) → return flow response or 204 on timeout. +- **Sync**: create FlowRun with `WEBHOOK_RESPONSE` → register `engineResponseWatcher` → wait (`AP_WEBHOOK_TIMEOUT_SECONDS`, default 30; MCP overrides it with `AP_FLOW_TIMEOUT_SECONDS`) → return the flow response, **500** if the run ended in a failure status, or **408** `REQUEST_TIMEOUT` when nothing answered at all. This route's default was **204** `NO_CONTENT` until 0.86.0 (#13909) changed it to 408; the waitpoint sync resume route still defaults to 204. - **Version resolution** `LOCKED_FALL_BACK_TO_LATEST`: uses `publishedVersionId` if set, else latest draft. - **Payload normalization** (`convertRequest`): multipart parts and binary bodies upload to the File service and the payload carries URLs; JSON/text pass through. `BINARY_CONTENT_TYPE_PATTERNS` covers `image/*`, `video/*`, `audio/*`, `application/pdf|zip|gzip|octet-stream` and `text/csv` (each also needs a `addContentTypeParser` entry in `webhook-module.ts` to stream rather than parse). Subflow linkage is read off `x-parent-run-id` / `x-fail-parent-on-failure`. @@ -30,6 +30,14 @@ Webhooks are the primary entry point for event-driven flow execution from outsid - **Size guard**: `AP_MAX_WEBHOOK_PAYLOAD_SIZE_MB` (default 5MB) → 413. Raw-binary bodies pipe through `enforceByteLimit`; oversized multipart parts are failed at end-of-stream (busboy flags `truncated` cleanly rather than erroring). - **Handshake** runs BEFORE the disabled-flow guard, so ownership pings work both during the publish window and for re-verification on enabled flows. Strategies: `HEADER_PRESENT`, `QUERY_PRESENT`, `BODY_PARAM_PRESENT`, `NONE`, `HEAD_REQUEST` (e.g. Trello). - Flow resolution returns 410 GONE if not found; 404 if disabled (unless the request matches the flow's handshake config). +- **Two things answer a sync webhook, and they answer for different reasons.** A piece hook answers with the flow's own response: `piece-executor.ts` posts `sendFlowResponse` when a step returns a `respond`/`stopped`/`paused` hook response *and* that step's piece matches `constants.triggerPieceName`. A terminal failure status answers with 500: `engineRunCallbackService.uploadRunLog` publishes to `engine-run:sync:` when the reported status is `FAILED`, `INTERNAL_ERROR`, `TIMEOUT`, `MEMORY_LIMIT_EXCEEDED` or `LOG_SIZE_EXCEEDED` and the request carries both correlation ids. Everything else still falls through to the listener's 408 default, which now means only two things: the run is still going, or it succeeded without reaching a Return Response step. +- **Of the five failure statuses the gate answers, `TIMEOUT` is the one that almost never reaches a caller.** A run only becomes `TIMEOUT` when the worker kills the sandbox at `AP_FLOW_TIMEOUT_SECONDS` (default 600), while the sync listener gives up at `AP_WEBHOOK_TIMEOUT_SECONDS` (default 30), so on a default install the caller has had its 408 twenty times over and the 500 publish lands on a deleted listener as a no-op. It pays off in three configurations only: a webhook timeout raised above the flow timeout (the docs allow up to 15 minutes against a 600s flow default), a flow timeout lowered below the webhook timeout, and the MCP `runFlowAsTool` path, which waits exactly `AP_FLOW_TIMEOUT_SECONDS` and so ties with the run's own timeout. The worker-reported statuses that actually pay off inside a default 30s window are `INTERNAL_ERROR` (the sandbox crash sampled on cloud died in 3.25s) and `MEMORY_LIMIT_EXCEEDED` (reproducibly ~8s with fat piece bundles, see [workers](../execution-runtime/workers.md)). +- **The 500 gate lives in the app, not in the engine or the worker, because neither can cover the other's failures.** The engine's `FlowVerdict` type can only ever be `PAUSED`, `SUCCEEDED`, `FAILED | LOG_SIZE_EXCEEDED` or `RUNNING`, so an engine-side gate physically cannot report `INTERNAL_ERROR`, `TIMEOUT` or `MEMORY_LIMIT_EXCEEDED`; those are detected by the worker from how the sandbox died. A worker-side gate misses the common case, an ordinary step failure, which the engine reports itself while the worker's job still ends `success`. `POST /v1/engine/run-logs` is the one choke point both post through, so the gate sits there and both senders just carry `workerHandlerId` and `httpRequestId` on the request. +- **A sync webhook used to hang the full timeout on a failed run from 0.80.0 until this fix, answering 204 for the first six releases and 408 after that.** Worker v2 (#11608) deleted `sandbox-event-handlers.ts`, which had published a mapped response on every terminal-and-not-`RUNNING`/`SUCCEEDED`/`PAUSED` run-log upload (`FAILED` and `MEMORY_LIMIT_EXCEEDED` gave 500, `INTERNAL_ERROR` 500, `TIMEOUT` 504, `QUOTA_EXCEEDED` 204). `LOG_SIZE_EXCEEDED` and `CANCELED` existed then but had no case in `getFlowResponse`, so they hit its `default: throw`, which fired *before* `runsMetadataQueue.add` and cost both the response and the metadata write; `LOG_SIZE_EXCEEDED` therefore gets a real 500 now for the first time in any release, so the fix is not a pure restoration, and `UploadRunLogsRequest` lost its two correlation ids in the same refactor, which is why nothing on the app side could answer. The listener's own default then decided the reply, and it was `StatusCodes.NO_CONTENT` in `webhook-handler.ts` until #13909 (0.86.0) switched it to `REQUEST_TIMEOUT`, so a failed run answered a **204 that reads as success to most HTTP clients** through 0.80 to 0.85, and 408 from 0.86 on. At its peak on cloud 0.88.3 this was 7,962 of 20,536 sync calls in 24h (38.8%) across 133 platforms, every one sitting at exactly 30.0s while the run had finished ~27s earlier. `SUCCEEDED` with no Return Response step blocked for the full timeout back then too, so that half was never a regression and is still 408 today. +- **`workerHandlerId` and `httpRequestId` are persisted in exactly one place: `waitpoint.workerHandlerId` and `waitpoint.httpRequestId`, and only for paused runs.** `flow_run` has no such column and no migration ever added one, so for a plain sync run the two ids live only in the BullMQ job payload. That is why answering a waiting caller from the app side means carrying them back on the request rather than looking them up. A useful consequence of the waitpoint columns: an async resume inherits them (`resume-service.ts`), so a resumed run that then fails answers the original sync caller too. +- **Splitting a sync timeout into engine-class and worker-class tells you who can still answer.** Joining timed-out sync runs to their worker `job.execute` event over 3h gave 5,685 with outcome `success` (the engine reported the terminal status itself) against 141 `failed` (137 `SANDBOX_INTERNAL_ERROR`, the rest RPC timeout or socket exit). Note no `flowRun.status` attribute is shipped to ClickHouse, so separating a genuinely failed run from a silent success inside that 5,685 needs a Postgres query on `flow_run.status`. +- **The sync response reaches the caller before the run row says it failed.** `uploadRunLog` only *enqueues* the status onto `runsMetadataQueue`; the Postgres write happens later in that queue's worker, behind a distributed lock. The 500 is published in the same call, so a caller that gets its 500 and immediately reads the run over the API can still see `RUNNING`. Do not assert the persisted status straight after a sync response in a test: poll for it. An e2e assertion written that way failed on exactly this while the response itself was already correct. +- **A retry can never answer a sync caller.** Flow jobs run `attempts: 2` with exponential backoff starting at eight minutes (`job-queue.ts`), so the second attempt lands long after any 30s caller is gone. Publishing a 500 the moment a run fails therefore costs nothing, and waiting for a retry to maybe succeed would buy nothing. ### Editions Full functionality in CE/EE/Cloud; Cloud makes payload size and timeout configurable per environment. diff --git a/brain/knowledge/execution-runtime/index.md b/brain/knowledge/execution-runtime/index.md index 899ef70ff7c8..52d9e1a39a23 100644 --- a/brain/knowledge/execution-runtime/index.md +++ b/brain/knowledge/execution-runtime/index.md @@ -54,6 +54,8 @@ The four calls a run emits to the app during execution: `updateRunProgress`, `up 📁 **Decisions nested under this page:** *Worker is the Sandbox* · *Transitional multi-box concurrency* · *Engine posts run-time callbacks directly* · *Sandbox pool is a pure execute() (superseded)* · *Freeze piece versions in the Flow Bundle manifest*. +- **`SANDBOX_INTERNAL_ERROR` is the residual bucket, not a diagnosis.** It is raised in exactly two places in `sandbox.ts`: `createSocketServer` failing to bind the worker ws port after its retries (the engine never started), and the child-process exit branch that runs *after* the three attributable causes have been ruled out (killed-by-timeout gives `SANDBOX_EXECUTION_TIMEOUT`, OOM gives `SANDBOX_MEMORY_ISSUE` via the heap-OOM string / code 134 / SIGABRT / an ambiguous SIGKILL outside shutdown, and the log ceiling gives `SANDBOX_LOG_SIZE_EXCEEDED`). Everything left becomes `Worker exited with code and signal `, which is why the message is opaque and `fork.ts` says so in a comment. A real instance seen on cloud: `code 1, signal null` with the engine's own stderr `[engine] Worker socket disconnected (ping timeout), exiting`, meaning the engine gave up on a silent socket and exited itself. When you see this the engine is gone, so nothing engine-side reported the run: the *worker* marks the run `INTERNAL_ERROR` through `reportFlowStatus`, and that is the only signal downstream (a waiting sync caller included) ever gets. + ## Pages - **Workers** — the poll loop, worker groups, slots and reservations, and its gotchas: the version gate, system-job edition skew, `kamal app exec` leaking a permanent worker, serial per-queue dispatch as the real throughput cap, the silent mid-poll-loop wedge, and why polling starves first diff --git a/brain/knowledge/flows-execution/flow-runs.md b/brain/knowledge/flows-execution/flow-runs.md index 516f1ab04e98..1bb6374a7f42 100644 --- a/brain/knowledge/flows-execution/flow-runs.md +++ b/brain/knowledge/flows-execution/flow-runs.md @@ -21,6 +21,7 @@ A Flow Run records one execution of a specific flow version, from trigger to ter - **RUN_TELEMETRY job**: `flow-run-module.ts` registers a BullMQ system job (cron `50 23 * * *`, once daily at 23:50 UTC) that aggregates the day's run counts by `(projectId, flowId, environment)` in one transaction (5-minute statement timeout) and emits a `FLOW_RUN_CREATED` telemetry event per group. No-op when telemetry is disabled. The cron was `0/50 23 * * *` until GIT-1632, which also fired at 23:00 with partial counts. ### Gotchas +- **A Delay inside a Loop pauses and requeues the whole run once per iteration, so no fixed sync-webhook timeout can cover it.** The delay is not a sleep inside the step: each iteration arms a waitpoint, the run goes `PAUSED`, and the resume comes back through the queue, so every item costs its delay plus queue latency and the total scales with the item count. Worked case on dev: `Catch Webhook → Code → Loop { Delay For 8s } → Return Response` over 6 items reported `stepsCount` 9 (Code + Loop + 6 delays + Return Response) and ran 53.6s, of which 48s was delay and 3s was the initial queue leg. With `AP_WEBHOOK_TIMEOUT_SECONDS` at 30 the `/sync` caller was answered 408 mid-loop; Return Response then ran ~23s later and published to a listener that had already resolved and been deleted, so it was a no-op. Putting the response step *after* slow work is the bug: respond before it (respond-and-continue rather than `stop`) or go async with a callback, because raising the timeout only works until someone sends more items. See the sync-response gotchas in [webhooks](../eventing-webhooks/webhooks.md). - **A worker OOM-kill leaves the run stuck in RUNNING forever, and Cancel is greyed out.** The flow timeout is enforced *inside* the worker, so if the pod dies (OOM) nothing ever transitions the run to a terminal state; Cancel only applies to paused/queued runs, so the UI offers no way out and the run can't be retried either. Bug: activepieces#14372, fix PR #14374. Manual unblock on the customer's Postgres: `UPDATE flow_run SET status = 'CANCELED', "finishTime" = NOW(), updated = NOW() WHERE id = '' AND status = 'RUNNING';` (run id = last path segment of the run URL), then "Retry on latest version" replays the original payload. - **Resume Confirmation Page (scanner guard)**: the `/confirm` route serves an HTML Approve/Disapprove page on `GET`/`HEAD` (never consumes) and only resumes on `POST` — stops email security scanners (Safe Links, Mimecast, Proofpoint) prefetching approval links. The deprecated bare `GET /:id/waitpoints/:waitpointId` still resumes for old emails. Slack is unchanged (server-side POST from webhook). - **Cross-project isolation (subflow parent-fail)**: `markParentRunAsFailed` scopes its parent lookup to `{ id: parentRunId, projectId }` using the child run's authenticated `projectId`. `parentRunId`/`failParentOnFailure` arrive from spoofable webhook headers (`ap-parent-run-id`/`ap-fail-parent-on-failure`) on the public webhook endpoint, so without the scope a failed child in project A could complete a paused parent's waitpoint and resume it in project B. A cross-project parent id now matches nothing and the fail is a no-op; legitimate subflows are always same-project (Call Flow only targets flows in the caller's project). diff --git a/bun.lock b/bun.lock index 17e5a5103503..8ed1d4b8a2d4 100644 --- a/bun.lock +++ b/bun.lock @@ -118,7 +118,7 @@ }, "packages/core/execution": { "name": "@activepieces/core-execution", - "version": "0.18.0", + "version": "0.18.1", "dependencies": { "@activepieces/core-piece-types": "workspace:*", "@activepieces/core-utils": "workspace:*", @@ -163,7 +163,7 @@ }, "packages/core/shared": { "name": "@activepieces/shared", - "version": "0.155.0", + "version": "0.156.0", "dependencies": { "@activepieces/core-execution": "workspace:*", "@activepieces/core-formula": "workspace:*", diff --git a/docs/install/reference/breaking-changes.mdx b/docs/install/reference/breaking-changes.mdx index aea1a2b5f6a5..afa1b0365474 100644 --- a/docs/install/reference/breaking-changes.mdx +++ b/docs/install/reference/breaking-changes.mdx @@ -143,6 +143,20 @@ This affects the Tables piece's Find Records action and any direct API call that Nothing on upgrade. Re-check any flow or API integration that filters a Date column with `gt`, `gte`, `lt` or `lte` — it now returns the rows the filter actually describes, which may be more or fewer than before. +#### A failed run on a synchronous webhook answers with 500 instead of 408 after the full timeout + +A `/sync` webhook call whose flow run fails used to hold the connection open for the whole `AP_WEBHOOK_TIMEOUT_SECONDS` (30 seconds by default) and then answer `408 Request Timeout` with an empty body, no matter how quickly the run had actually failed. Nothing reported a terminal run status back to the waiting request, so the caller only ever saw the timeout fallback. + +A run that ends in `FAILED`, `INTERNAL_ERROR`, `TIMEOUT`, `MEMORY_LIMIT_EXCEEDED` or `LOG_SIZE_EXCEEDED` now answers immediately with `500` and a body of `{"message": "The flow has failed and there is no response returned"}`. For `FAILED`, `INTERNAL_ERROR` and `MEMORY_LIMIT_EXCEEDED` this restores the behaviour from before 0.80.0, where they answered `500` (`TIMEOUT` answered `504` then; it is `500` now, like the rest). `LOG_SIZE_EXCEEDED` never had a response of its own in any earlier release, so it gains one here. + +Between 0.80.0 and 0.85.x the same failures answered `204 No Content` rather than `408`, which many HTTP clients read as success. If you integrated against an Activepieces in that range and concluded a synchronous webhook "returns 204 when it fails", that is the behaviour being replaced. + +Two cases are deliberately unchanged. A run that succeeds without reaching a Return Response step still waits out the timeout and answers `408`, exactly as it did before 0.80.0. A run blocked on credits still answers `402` before it starts. + +#### What you need to do + +Nothing to configure or migrate. If you have a caller or reverse proxy that treats `408` as the signal for a failed synchronous flow, or retry logic keyed on `408`, switch it to `5xx` — a failed run no longer produces a `408`, and the reply now arrives in seconds rather than after the timeout. + ## 0.88.2 ### What has changed? diff --git a/docs/install/reference/limits.mdx b/docs/install/reference/limits.mdx index 1bcba634905d..81f6a16c8efc 100644 --- a/docs/install/reference/limits.mdx +++ b/docs/install/reference/limits.mdx @@ -115,10 +115,13 @@ incoming payload can be. | Webhook payload inline threshold | 1024 KB | `AP_WEBHOOK_PAYLOAD_INLINE_THRESHOLD_KB` | `512` | -For synchronous webhook requests (URLs ending in `/sync`), Activepieces will -wait up to the response timeout before returning HTTP 408. Payloads above the -inline threshold are offloaded from Redis to file storage to protect Redis -memory; smaller payloads stay inline for the fastest path. +For synchronous webhook requests (URLs ending in `/sync`), a run that fails +answers straight away with HTTP 500. HTTP 408 is returned only when the run +never produced a response at all within the timeout, which is the case for a +flow that is still running or one that finishes without a Return Response +step. Payloads above the inline threshold are offloaded from Redis to file +storage to protect Redis memory; smaller payloads stay inline for the fastest +path. --- diff --git a/packages/core/execution/package.json b/packages/core/execution/package.json index 2a77d8a8bb31..beb19d2228d3 100644 --- a/packages/core/execution/package.json +++ b/packages/core/execution/package.json @@ -1,6 +1,6 @@ { "name": "@activepieces/core-execution", - "version": "0.18.0", + "version": "0.18.1", "type": "commonjs", "main": "./dist/src/index.js", "scripts": { diff --git a/packages/core/execution/src/lib/engine/requests.ts b/packages/core/execution/src/lib/engine/requests.ts index 385ce6901421..3e1faf8ef2f6 100644 --- a/packages/core/execution/src/lib/engine/requests.ts +++ b/packages/core/execution/src/lib/engine/requests.ts @@ -27,6 +27,8 @@ export const UploadRunLogsRequest = z.object({ provisionMs: z.number().optional(), bootMs: z.number().optional(), runMs: z.number().optional(), + workerHandlerId: z.string().optional(), + httpRequestId: z.string().optional(), }) export type UploadRunLogsRequest = z.infer diff --git a/packages/server/api/src/app/flows/flow-run/engine-run-callback-service.ts b/packages/server/api/src/app/flows/flow-run/engine-run-callback-service.ts index 1786d2d50dd5..2f1287c05e1b 100644 --- a/packages/server/api/src/app/flows/flow-run/engine-run-callback-service.ts +++ b/packages/server/api/src/app/flows/flow-run/engine-run-callback-service.ts @@ -1,6 +1,7 @@ import { isNil, tryCatch } from '@activepieces/core-utils' -import { ApEdition, ExecutioOutputFile, FileCompression, FileType, isFlowRunStateTerminal, logSerializer, RunInternalError, RunInternalErrorSource, SendFlowResponseRequest, StreamStepProgress, truncateFailedStepMessage, UpdateStepProgressRequest, UploadRunLogsRequest, WebsocketClientEvent } from '@activepieces/shared' +import { ApEdition, ExecutioOutputFile, FileCompression, FileType, FlowRunStatus, isFlowRunStateTerminal, logSerializer, RunInternalError, RunInternalErrorSource, SendFlowResponseRequest, StreamStepProgress, truncateFailedStepMessage, UpdateStepProgressRequest, UploadRunLogsRequest, WebsocketClientEvent } from '@activepieces/shared' import { FastifyBaseLogger } from 'fastify' +import { StatusCodes } from 'http-status-codes' import { websocketService } from '../../core/websockets.service' import { fileCompressor } from '../../file/file-compressor' import { fileService } from '../../file/file.service' @@ -55,6 +56,20 @@ export const engineRunCallbackService = (log: FastifyBaseLogger) => ({ } await runsMetadataQueue(log).add(logData) + if (!isNil(request.status) && FAILED_RUN_SYNC_STATUSES.includes(request.status) && !isNil(request.workerHandlerId) && !isNil(request.httpRequestId)) { + await engineRunCallbackService(log).sendFlowResponse({ + request: { + workerHandlerId: request.workerHandlerId, + httpRequestId: request.httpRequestId, + runResponse: { + status: StatusCodes.INTERNAL_SERVER_ERROR, + body: { message: FAILED_RUN_SYNC_MESSAGE }, + headers: {}, + }, + }, + }) + } + if (request.stepResponse && request.streamStepProgress === StreamStepProgress.WEBSOCKET) { const stepData = { ...request.stepResponse, projectId } if (!isTerminal) { @@ -67,6 +82,15 @@ export const engineRunCallbackService = (log: FastifyBaseLogger) => ({ }, }) +const FAILED_RUN_SYNC_STATUSES = [ + FlowRunStatus.FAILED, + FlowRunStatus.INTERNAL_ERROR, + FlowRunStatus.TIMEOUT, + FlowRunStatus.MEMORY_LIMIT_EXCEEDED, + FlowRunStatus.LOG_SIZE_EXCEEDED, +] +const FAILED_RUN_SYNC_MESSAGE = 'The flow has failed and there is no response returned' + async function ensureLogsFileExists({ log, projectId, logsFileId, internalError }: EnsureLogsFileParams): Promise { const { error } = await tryCatch(async () => { const fileExists = await fileService(log).exists({ diff --git a/packages/server/api/test/integration/ce/flows/flow-run/execute-flow-e2e.test.ts b/packages/server/api/test/integration/ce/flows/flow-run/execute-flow-e2e.test.ts index 0ad40d1164e2..51bfee484ed9 100644 --- a/packages/server/api/test/integration/ce/flows/flow-run/execute-flow-e2e.test.ts +++ b/packages/server/api/test/integration/ce/flows/flow-run/execute-flow-e2e.test.ts @@ -427,6 +427,107 @@ async function pollFlowRunToCompletion(flowRunId: string, projectId: string) { return result } +async function setupSyncWebhookFlow({ code, withReturnResponse }: { code: string, withReturnResponse: boolean }) { + const { mockProject } = await mockAndSaveBasicSetup() + + const webhookPiece = createMockPieceMetadata({ + name: '@activepieces/piece-webhook', + version: '0.1.29', + platformId: undefined, + packageType: PackageType.REGISTRY, + pieceType: PieceType.OFFICIAL, + }) + await databaseConnection().getRepository('piece_metadata').save([webhookPiece]) + + const returnResponseAction = { + type: FlowActionType.PIECE as const, + name: 'step_2', + displayName: 'Return Response', + valid: true, + settings: { + pieceName: '@activepieces/piece-webhook', + pieceVersion: '0.1.29', + actionName: 'return_response', + input: { + responseType: 'json', + respond: 'stop', + fields: { + status: 200, + headers: {}, + body: { echo: '{{step_1[\'output\'].echo}}' }, + }, + }, + propertySettings: {}, + errorHandlingOptions: {}, + }, + } + + const codeAction = { + type: FlowActionType.CODE as const, + name: 'step_1', + displayName: 'Work', + valid: true, + settings: { + sourceCode: { code, packageJson: '{}' }, + input: { message: '{{trigger[\'output\'].body.message}}' }, + errorHandlingOptions: {}, + }, + ...(withReturnResponse ? { nextAction: returnResponseAction } : {}), + } + + const flow = createMockFlow({ projectId: mockProject.id, status: FlowStatus.ENABLED }) + await db.save('flow', flow) + + const flowVersion = createMockFlowVersion({ + flowId: flow.id, + state: FlowVersionState.LOCKED, + trigger: { + type: FlowTriggerType.PIECE, + name: 'trigger', + displayName: 'Catch Webhook', + valid: true, + lastUpdatedDate: new Date().toISOString(), + settings: { + pieceName: '@activepieces/piece-webhook', + pieceVersion: '0.1.29', + triggerName: 'catch_webhook', + input: { authType: 'none' }, + propertySettings: {}, + }, + nextAction: codeAction, + }, + }) + await db.save('flow_version', flowVersion) + await db.update('flow', flow.id, { publishedVersionId: flowVersion.id }) + + return flow +} + +const WEBHOOK_TIMEOUT_MS = Number(process.env.AP_WEBHOOK_TIMEOUT_SECONDS ?? 30) * 1000 +const FAILING_CODE = 'export const code = async () => { throw new Error(\'deliberate step failure\') }' +const WORKING_CODE = 'export const code = async (inputs) => ({ echo: inputs.message })' + +const waitForRunStatus = async (flowId: string, expected: FlowRunStatus) => { + for (let attempt = 0; attempt < 60; attempt++) { + const run = await databaseConnection().getRepository('flow_run').findOneBy({ flowId }) + if (run?.status === expected) { + return run.status + } + await new Promise((resolve) => setTimeout(resolve, 250)) + } + return (await databaseConnection().getRepository('flow_run').findOneBy({ flowId }))?.status +} + +const postSync = async (flowId: string) => { + const startedAt = Date.now() + const response = await app.inject({ + method: 'POST', + url: `/api/v1/webhooks/${flowId}/sync`, + payload: { message: 'hello world' }, + }) + return { response, elapsedMs: Date.now() - startedAt } +} + describe('Execute Flow E2E', () => { it('executes a webhook → data mapper → code flow end-to-end', async () => { const { mockPlatform, mockProject } = await mockAndSaveBasicSetup() @@ -1125,4 +1226,48 @@ describe('Execute Flow E2E', () => { expect(response.statusCode).toBe(200) expect(response.json()).toEqual(expect.objectContaining({ echo: 'hello world' })) }, 180_000) + it('answers a failed run with 500 instead of waiting out the webhook timeout', async () => { + const flow = await setupSyncWebhookFlow({ code: FAILING_CODE, withReturnResponse: false }) + + const { response, elapsedMs } = await postSync(flow.id) + + expect(response.statusCode).toBe(StatusCodes.INTERNAL_SERVER_ERROR) + expect(response.json()).toEqual({ message: 'The flow has failed and there is no response returned' }) + expect(elapsedMs).toBeLessThan(WEBHOOK_TIMEOUT_MS) + + expect(await waitForRunStatus(flow.id, FlowRunStatus.FAILED)).toBe(FlowRunStatus.FAILED) + }, 180_000) + + it('answers a successful run through its Return Response step with 200', async () => { + const flow = await setupSyncWebhookFlow({ code: WORKING_CODE, withReturnResponse: true }) + + const { response, elapsedMs } = await postSync(flow.id) + + expect(response.statusCode).toBe(StatusCodes.OK) + expect(response.json()).toEqual({ echo: 'hello world' }) + expect(elapsedMs).toBeLessThan(WEBHOOK_TIMEOUT_MS) + }, 180_000) + + it('still waits out the timeout and answers 408 when a successful run sends no response', async () => { + const flow = await setupSyncWebhookFlow({ code: WORKING_CODE, withReturnResponse: false }) + + const { response } = await postSync(flow.id) + + expect(response.statusCode).toBe(StatusCodes.REQUEST_TIMEOUT) + + expect(await waitForRunStatus(flow.id, FlowRunStatus.SUCCEEDED)).toBe(FlowRunStatus.SUCCEEDED) + }, 180_000) + + it('answers an async webhook with 200 as soon as it is queued', async () => { + const flow = await setupSyncWebhookFlow({ code: FAILING_CODE, withReturnResponse: false }) + + const response = await app.inject({ + method: 'POST', + url: `/api/v1/webhooks/${flow.id}`, + payload: { message: 'hello world' }, + }) + + expect(response.statusCode).toBe(StatusCodes.OK) + expect(response.headers['x-webhook-id']).toBeDefined() + }, 180_000) }) diff --git a/packages/server/api/test/unit/app/flows/flow-run/engine-run-callback-failed-sync-response.test.ts b/packages/server/api/test/unit/app/flows/flow-run/engine-run-callback-failed-sync-response.test.ts new file mode 100644 index 000000000000..67b92911708f --- /dev/null +++ b/packages/server/api/test/unit/app/flows/flow-run/engine-run-callback-failed-sync-response.test.ts @@ -0,0 +1,113 @@ +import { FlowRunStatus } from '@activepieces/shared' +import { StatusCodes } from 'http-status-codes' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockPublish, mockRunsMetadataAdd } = vi.hoisted(() => ({ + mockPublish: vi.fn(), + mockRunsMetadataAdd: vi.fn(), +})) + +vi.mock('../../../../../src/app/helper/pubsub', () => ({ + pubsub: { publish: mockPublish, subscribe: vi.fn(), unsubscribe: vi.fn() }, +})) + +vi.mock('../../../../../src/app/flows/flow-run/flow-runs-queue', () => ({ + runsMetadataQueue: () => ({ add: mockRunsMetadataAdd }), +})) + +vi.mock('../../../../../src/app/helper/system/system', () => ({ + system: { getEdition: vi.fn().mockReturnValue('cloud') }, +})) + +vi.mock('../../../../../src/app/core/websockets.service', () => ({ + websocketService: { to: () => ({ emit: vi.fn() }) }, +})) + +vi.mock('../../../../../src/app/file/file.service', () => ({ + fileService: () => ({ exists: vi.fn(), getDataOrUndefined: vi.fn(), save: vi.fn() }), +})) + +vi.mock('../../../../../src/app/file/file-compressor', () => ({ + fileCompressor: { compress: vi.fn() }, +})) + +vi.mock('../../../../../src/app/project/project-service', () => ({ + projectService: () => ({ getPlatformId: vi.fn() }), +})) + +const { engineRunCallbackService } = await import('../../../../../src/app/flows/flow-run/engine-run-callback-service') + +const noopLogger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } + +const uploadRunLog = (status: FlowRunStatus, ids?: { workerHandlerId?: string, httpRequestId?: string }) => + engineRunCallbackService(noopLogger as never).uploadRunLog({ + projectId: 'proj-1', + request: { + runId: 'run-1', + projectId: 'proj-1', + status, + finishTime: new Date().toISOString(), + ...ids, + }, + }) + +describe('uploadRunLog answering a waiting sync request', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it.each([ + FlowRunStatus.FAILED, + FlowRunStatus.INTERNAL_ERROR, + FlowRunStatus.TIMEOUT, + FlowRunStatus.MEMORY_LIMIT_EXCEEDED, + FlowRunStatus.LOG_SIZE_EXCEEDED, + ])('publishes a 500 for %s instead of leaving the caller to time out', async (status) => { + await uploadRunLog(status, { workerHandlerId: 'server-1', httpRequestId: 'req-1' }) + + expect(mockPublish).toHaveBeenCalledTimes(1) + const [channel, message] = mockPublish.mock.calls[0] + expect(channel).toBe('engine-run:sync:server-1') + expect(JSON.parse(message)).toEqual({ + requestId: 'req-1', + response: { + status: StatusCodes.INTERNAL_SERVER_ERROR, + body: { message: 'The flow has failed and there is no response returned' }, + headers: {}, + }, + }) + }) + + it.each([ + FlowRunStatus.SUCCEEDED, + FlowRunStatus.PAUSED, + FlowRunStatus.RUNNING, + FlowRunStatus.QUEUED, + FlowRunStatus.QUOTA_EXCEEDED, + FlowRunStatus.CANCELED, + ])('stays silent for %s so a respond step or the caller default still decides', async (status) => { + await uploadRunLog(status, { workerHandlerId: 'server-1', httpRequestId: 'req-1' }) + + expect(mockPublish).not.toHaveBeenCalled() + }) + + it('stays silent for a failed async run that has no waiting caller', async () => { + await uploadRunLog(FlowRunStatus.FAILED) + + expect(mockPublish).not.toHaveBeenCalled() + }) + + it('stays silent when only one of the two correlation ids is present', async () => { + await uploadRunLog(FlowRunStatus.FAILED, { httpRequestId: 'req-1' }) + await uploadRunLog(FlowRunStatus.FAILED, { workerHandlerId: 'server-1' }) + + expect(mockPublish).not.toHaveBeenCalled() + }) + + it('still records the run metadata when it answers', async () => { + await uploadRunLog(FlowRunStatus.FAILED, { workerHandlerId: 'server-1', httpRequestId: 'req-1' }) + + expect(mockRunsMetadataAdd).toHaveBeenCalledTimes(1) + expect(mockRunsMetadataAdd.mock.calls[0][0]).toMatchObject({ id: 'run-1', status: FlowRunStatus.FAILED }) + }) +}) diff --git a/packages/server/engine/src/lib/helper/flow-run-progress-reporter.ts b/packages/server/engine/src/lib/helper/flow-run-progress-reporter.ts index 618c1920ee5c..2b5805d00600 100644 --- a/packages/server/engine/src/lib/helper/flow-run-progress-reporter.ts +++ b/packages/server/engine/src/lib/helper/flow-run-progress-reporter.ts @@ -143,6 +143,8 @@ export const flowRunProgressReporter = { finishTime: isTerminal ? dayjs().toISOString() : undefined, tags: Array.from(flowExecutorContext.tags), stepsCount: flowExecutorContext.stepsCount, + workerHandlerId: engineConstants.workerHandlerId ?? undefined, + httpRequestId: engineConstants.httpRequestId ?? undefined, } await sendLogsUpdate({ engineConstants, request }) }) diff --git a/packages/server/worker/src/lib/execute/jobs/execute-flow.ts b/packages/server/worker/src/lib/execute/jobs/execute-flow.ts index c034120123ae..432976f2944d 100644 --- a/packages/server/worker/src/lib/execute/jobs/execute-flow.ts +++ b/packages/server/worker/src/lib/execute/jobs/execute-flow.ts @@ -1,5 +1,5 @@ import { inspect } from 'node:util' -import { ActivepiecesError, ErrorCode, isNil, tryCatch } from '@activepieces/core-utils' +import { ActivepiecesError, ErrorCode, isNil, spreadIfDefined, tryCatch } from '@activepieces/core-utils' import { onCallService } from '@activepieces/server-utils' import { BeginExecuteFlowOperation, EngineOperationType, EngineResponseStatus, ExecuteFlowJobData, ExecutionType, FailedStep, FlowRunStatus, FlowVersion, ResumeExecuteFlowOperation, RunInternalError, RunInternalErrorSource, WorkerJobType } from '@activepieces/shared' import { system, WorkerSystemProp } from '../../config/configs' @@ -175,6 +175,8 @@ async function reportFlowStatus({ ctx, data, status, internalError, failedStep } ...(isNil(internalError) ? {} : { logsFileId: data.logsFileId }), internalError, failedStep, + ...spreadIfDefined('workerHandlerId', data.workerHandlerId ?? undefined), + ...spreadIfDefined('httpRequestId', data.httpRequestId), }) if (status === FlowRunStatus.INTERNAL_ERROR && isDedicatedWorker()) { diff --git a/packages/server/worker/test/lib/execute/jobs/execute-flow.test.ts b/packages/server/worker/test/lib/execute/jobs/execute-flow.test.ts index 8b075d81f0ea..572127238c3d 100644 --- a/packages/server/worker/test/lib/execute/jobs/execute-flow.test.ts +++ b/packages/server/worker/test/lib/execute/jobs/execute-flow.test.ts @@ -205,4 +205,88 @@ describe('executeFlowJob', () => { expect(ctx.runtime.execute).not.toHaveBeenCalled() }) }) + describe('correlation ids on a terminal status report', () => { + const syncJobData = (overrides?: Partial) => makeResumeJobData({ + executionType: ExecutionType.BEGIN, + workerHandlerId: 'server-1', + httpRequestId: 'req-1', + ...overrides, + }) + + const sandboxError = (code: ErrorCode) => new ActivepiecesError({ + code, + params: { standardOutput: '', standardError: '' }, + }) + + it.each([ + [ErrorCode.SANDBOX_EXECUTION_TIMEOUT, FlowRunStatus.TIMEOUT], + [ErrorCode.SANDBOX_MEMORY_ISSUE, FlowRunStatus.MEMORY_LIMIT_EXCEEDED], + [ErrorCode.SANDBOX_LOG_SIZE_EXCEEDED, FlowRunStatus.LOG_SIZE_EXCEEDED], + ])('reports %s as %s with both ids so the waiting sync caller can be answered', async (code, status) => { + const ctx = makeMockContext() + ctx.runtime.execute = vi.fn().mockRejectedValue(sandboxError(code)) + + await executeFlowJob.execute(ctx, syncJobData()) + + expect(ctx.apiClient.uploadRunLog).toHaveBeenCalledWith( + expect.objectContaining({ status, workerHandlerId: 'server-1', httpRequestId: 'req-1' }), + ) + }) + + it('reports an engine INTERNAL_ERROR with both ids', async () => { + const ctx = makeMockContext() + ctx.runtime.execute = vi.fn().mockResolvedValue({ status: EngineResponseStatus.INTERNAL_ERROR, error: 'boom', timings: {} }) + + await executeFlowJob.execute(ctx, syncJobData()) + + expect(ctx.apiClient.uploadRunLog).toHaveBeenCalledWith( + expect.objectContaining({ + status: FlowRunStatus.INTERNAL_ERROR, + workerHandlerId: 'server-1', + httpRequestId: 'req-1', + }), + ) + }) + + it('reports a sandbox crash as INTERNAL_ERROR with both ids before rethrowing', async () => { + const ctx = makeMockContext() + ctx.runtime.execute = vi.fn().mockRejectedValue(new Error('SANDBOX_INTERNAL_ERROR')) + + await expect(executeFlowJob.execute(ctx, syncJobData())).rejects.toThrow('SANDBOX_INTERNAL_ERROR') + + expect(ctx.apiClient.uploadRunLog).toHaveBeenCalledWith( + expect.objectContaining({ + status: FlowRunStatus.INTERNAL_ERROR, + workerHandlerId: 'server-1', + httpRequestId: 'req-1', + }), + ) + }) + + it('reports a vanished flow version as FAILED with both ids', async () => { + const ctx = makeMockContext({ resolveResult: { kind: 'flow-not-found' } }) + + await executeFlowJob.execute(ctx, syncJobData()) + + expect(ctx.apiClient.uploadRunLog).toHaveBeenCalledWith( + expect.objectContaining({ + status: FlowRunStatus.FAILED, + workerHandlerId: 'server-1', + httpRequestId: 'req-1', + }), + ) + }) + + it('omits both ids for an async run so nothing is published for it', async () => { + const ctx = makeMockContext() + ctx.runtime.execute = vi.fn().mockRejectedValue(sandboxError(ErrorCode.SANDBOX_EXECUTION_TIMEOUT)) + + await executeFlowJob.execute(ctx, makeResumeJobData({ executionType: ExecutionType.BEGIN })) + + const reported = ctx.apiClient.uploadRunLog.mock.calls.at(-1)[0] + expect(reported.status).toBe(FlowRunStatus.TIMEOUT) + expect(reported).not.toHaveProperty('workerHandlerId') + expect(reported).not.toHaveProperty('httpRequestId') + }) + }) }) From 67a3e3b4a76f9d8558b83a3b54dfb86c58627c43 Mon Sep 17 00:00:00 2001 From: Chaker Atallah <74781393+MrChaker@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:45:05 +0100 Subject: [PATCH 3/3] chore(release): v0.90.0 (#15229) Co-authored-by: Othman Emad Co-authored-by: Claude Opus 5 (1M context) --- docker-compose.yml | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 83797579e55c..14254b283d8c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,6 +1,6 @@ services: app: - image: ghcr.io/activepieces/activepieces:0.89.0 + image: ghcr.io/activepieces/activepieces:0.90.0 container_name: activepieces-app restart: unless-stopped ports: @@ -16,7 +16,7 @@ services: networks: - activepieces worker: - image: ghcr.io/activepieces/activepieces:0.89.0 + image: ghcr.io/activepieces/activepieces:0.90.0 restart: unless-stopped depends_on: - app diff --git a/package.json b/package.json index a52c64cdd3a8..9a151a087a50 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "activepieces", - "version": "0.89.0", + "version": "0.90.0", "packageManager": "bun@1.4.0", "trustedDependencies": [ "sqlite3",