diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c8f3442b1e23..15baa200de48 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -149,7 +149,7 @@ jobs: set -euo pipefail pids=() - npx turbo run test --filter=@activepieces/engine --filter=@activepieces/shared --filter=@activepieces/sandbox --filter=@activepieces/ai-providers --filter=@activepieces/pieces-framework & + npx turbo run test --filter=@activepieces/engine --filter=@activepieces/shared --filter=@activepieces/sandbox --filter=@activepieces/ai-providers --filter=@activepieces/pieces-framework --filter=web & pids+=($!) npx turbo run test-ce test-ee test-cloud check-migrations --filter=api & diff --git a/brain/knowledge/data-storage-observability/file-storage.md b/brain/knowledge/data-storage-observability/file-storage.md index e22efd1093cd..9df5fd9b027e 100644 --- a/brain/knowledge/data-storage-observability/file-storage.md +++ b/brain/knowledge/data-storage-observability/file-storage.md @@ -36,6 +36,7 @@ The central service for persisting binary files, backing the execution engine an - **The live piece-bundle cache sits *inside* the legacy one — `pieces/v2/` is nested under `pieces/`, so a recursive delete of `pieces/` takes the active cache with it.** `S3_PIECES_PREFIX` in `piece-bundle.ts` is `pieces/v2/`; the bare `pieces/` keys beside it are pre-CDN tarballs left by the older writer. Combined with the doubled prefix above, the real keys are `ap-files-prod/pieces/…` (legacy) and `ap-files-prod/pieces/v2/…` (live). Probe both with `wrangler r2 object get` before any prefix-wide operation — wiping v2 used to be survivable because it refilled lazily, at the cost of a burst of cache misses on every piece. **Both prefixes are now dead storage and safe to sweep:** the `BUNDLE_PIECE` job and the S3 mirror were removed, so `resolve()` no longer reads or writes either prefix and registry pieces redirect straight to the CDN (else npm). The mirror was deleted because it was written from whichever source was preferred *at cache time* and then took precedence over the CDN forever — a bucket populated before the CDN became preferred kept serving the unbundled npm build, which is what fans out one `@activepieces/shared` copy per piece in the engine (see the Workers page). - **`deleteFiles` succeeding does not mean the objects are gone.** `DeleteObjectsCommand` reports per-object failures in `response.Errors` and does **not** throw, and `Quiet: true` only suppresses the success entries — so a request that "worked" can still have left objects behind. `deleteFiles` logs a warn naming the failure codes, which is all its callers (best-effort cleanup) need. Anything whose *correctness* depends on the prefix being empty afterwards would have to surface those keys and retry — but prefer not to need that at all: a reader that must not see the old objects should read from a new key prefix rather than race a delete against writers that may still be running old code. - Cleanup job runs hourly (`30 */1 * * *`), deletes stale execution files past `EXECUTION_DATA_RETENTION_DAYS`; processes ~4000/iteration, deletes S3 keys in batches of 100. +- **`deleteStaleBulk`'s SELECT needs an explicit `ORDER BY created` — or the planner picks a Seq Scan for high-cardinality types and blows `statement_timeout` every hour.** The composite index `idx_file_type_created_desc` covers `(type, created)`, and equality-per-type was chosen (over `type IN (…)`) specifically to hit it. But without an `ORDER BY`, PG's LIMIT-cost heuristic reasons that if a type is 10%+ of the table, seq-scanning ~10 heap rows should yield one hit, so cost 2032 for `LIMIT 4000` beats the ~3000 index-scan cost — then in reality the scan wades through dead-tuple bloat and never finishes. Seen Aug 2026 on cloud: `file` table 149M rows / 290 GB / 16.5M dead tuples, `WEBHOOK_PAYLOAD` ≈ 15% of it → **every hourly run timed out at 60 s** for a month, only that one type; other types (FLOW_RUN_LOG, FLOW_STEP_FILE, …) sat under 200 ms. Adding `ORDER BY created ASC` pinned the plan to the index and dropped the same query to ~500 ms. Also true for any future retention-style sweep of `file`: never trust the planner to reach for `(type, created)` from the WHERE alone. - **Retention cleanup is row-driven, so any S3 object written without a `file` row is immortal.** The job walks `file` rows and deletes each one's `s3Key`; it never lists the bucket. The piece-tarball cache was exactly that shape — keyed by `-.tgz` under a bare prefix, with no row anywhere — so nothing has ever swept it and nothing structurally could, whatever the retention setting says. Checked Aug 2026: no migration or job has ever bulk-deleted the `pieces/` prefix, and the only `deleteFiles` callers are the row-driven cleanup and the health probe's own key. If you add a store path that bypasses the `file` table, you own its lifecycle by hand — prefer writing a row, or expect a manual `wrangler`/`aws s3` operation forever. - S3 deletes send CRC32C checksum (OCI rejects the SDK-default CRC32). `S3_ENDPOINT` set → SDK checksum/aws-chunked encoding disabled for S3-compatible providers. - `S3_USE_SIGNED_URLS=true` redirects downloads to 7-day pre-signed URLs instead of streaming through the app. diff --git a/brain/knowledge/engineering/ci-pr-review-hygiene.md b/brain/knowledge/engineering/ci-pr-review-hygiene.md index cf32f8738963..91cf99bfaeba 100644 --- a/brain/knowledge/engineering/ci-pr-review-hygiene.md +++ b/brain/knowledge/engineering/ci-pr-review-hygiene.md @@ -28,3 +28,4 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def - **Retargeting a stacked PR to `main` does not drop its base branch — it merges the whole thing.** A PR opened against a long-lived feature branch shows a small diff *relative to that base*, but `gh pr edit --base main` only moves the target; the branch still contains every commit of its old base. [#14593](https://github.com/activepieces/activepieces/pull/14593) read as 2 docs files against `feat/autumn-billing-integration` and as 198 commits / 211 files / +12k lines against `main`. Check with `git diff --stat origin/main...` **before** retargeting, and if it disagrees with the PR page, cherry-pick that PR's own commits onto `main` and force-push instead. A "conflict" on such a PR is often against the feature base only — those same commits can apply to `main` cleanly. - **A decision authored on a long-lived branch will collide on its number.** `brain/decisions/` numbers are assigned once and never reused, but the next free number is only knowable against `main` — two branches in flight both grab it. #14593 carried a `000024` that `main` had since filled, and `000025` too, so it landed as `000026`. Renumber against `main` at merge time and update every referring link; nothing in CI catches a duplicate number or a dead decision link. - **`tools/scripts/` is outside the lint and test wiring.** ESLint ignores it, and `npm run test-unit` only covers engine/shared/web. A script there with real policy logic must run its own tests from its own workflow — `pr-size.yml` runs `bun test tools/scripts/pr-size-check.test.ts` as a step before the check itself. +- **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. diff --git a/brain/knowledge/engineering/web-feature-anatomy.md b/brain/knowledge/engineering/web-feature-anatomy.md index b1769884fc23..c8b2c46dc0f8 100644 --- a/brain/knowledge/engineering/web-feature-anatomy.md +++ b/brain/knowledge/engineering/web-feature-anatomy.md @@ -62,6 +62,7 @@ Verify with `npx turbo run lint --filter=web`, or `npm run lint-dev` for the who ## Gotchas +- **A `packages/web` test runs in the `node` environment by default, so importing anything that touches `window` at module load fails at collection.** `vitest.config.ts` sets `environment: 'node'`; ~26 suites opt into a DOM with a `// @vitest-environment jsdom` docblock on line 1. The failure is a bare `ReferenceError: window is not defined` pointing at a *transitive* import (`embed-provider.tsx` reading `window.opener`, reached via `@/features/projects`), not at the test — so read the stack, don't hunt in your own file. Missing the docblock is why `chunk-reducer.test.ts` was red for as long as it was: CI did not run the web suite at all, so nothing surfaced it. - **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 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. @@ -71,3 +72,4 @@ Verify with `npx turbo run lint --filter=web`, or `npm run lint-dev` for the who - **`Alert`'s `warning` and `destructive` variants ship without a background tint, so a tinted banner has to add one at the call site.** `components/ui/alert.tsx` gives `primary` and `success` a `bg-*-100/10` wash but leaves `warning` and `destructive` transparent (`destructive` sets `bg-card`, which reads as a plain panel on a page background, and unlike `warning` it sets no border colour either). A banner that needs to look like a banner rather than a bordered paragraph passes `bg-warning-100/10` / `bg-destructive-100/10 border-destructive/50` itself — that is what the credits usage alert does. Don't "fix" it in the variant without looking: eight-plus existing warning alerts sit inside dialogs on card backgrounds and were designed against the untinted look. Note also that `--warning-100` and `--destructive-100` are *not* redefined in the `.dark` block of `styles.css` (unlike `--primary-100`), so in dark mode both tints are a very pale hue at 10% over near-black — subtle by accident, not by design. - **`npx turbo run serve --filter=web -- --mode=cloud` cannot do OAuth2 connections.** The provider redirects to `cloud.activepieces.com` after sign-in instead of your local frontend. Use API-key or basic-auth connections, or run a fully local backend. - **`--mode=cloud` also floods the terminal with `[vite] http proxy error: /ingest/... ETIMEDOUT 127.0.0.1:3000`.** The mode only redirects the API (`API_BASE_URL` → `https://cloud.activepieces.com` in `lib/api.ts`); PostHog still posts to the *relative* `api_host: '/ingest'` (a same-origin reverse proxy so ad blockers don't drop ingestion — `providers/telemetry-provider.tsx`, mirrored in prod by the `fastifyHttpProxy` in `server.ts`). Vite proxies `/ingest` to `127.0.0.1:3000`, which isn't running. Cloud flags also turn telemetry *on* (`TELEMETRY_ENABLED` + `EDITION=cloud`), unlike a local CE backend — so posthog-js keeps polling `/ingest/flags` and flushing `/ingest/e` every few seconds. Harmless, but note the same setup sends real dev clicks to production PostHog whenever `/ingest` does resolve; the clean fix is skipping `posthog.init` under `import.meta.env.DEV`. +- **`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. diff --git a/brain/knowledge/flows-execution/formulas.md b/brain/knowledge/flows-execution/formulas.md index 6dde5c1a548b..71e42743758a 100644 --- a/brain/knowledge/flows-execution/formulas.md +++ b/brain/knowledge/flows-execution/formulas.md @@ -20,6 +20,9 @@ In-builder data transformation: users transform any text input using ~104 functi - No new HTTP endpoints, no DB tables, no worker job — function metadata is bundled in `@activepieces/shared` and read directly by the frontend; evaluation is synchronous inside the engine. - Evaluation failure throws `FormulaEvaluationError` (an `ExecutionError`), so the step fails with a structured message instead of crashing the engine. - Type checker skips expression-operator args (e.g. `3 == 9`) to avoid false-positive errors on runtime-evaluated values. +- **`tokenizeExpression` tracks string literals, and that is what eats reference chips.** The serializer in `text-input-utils.ts` enters string mode on any `"`/`'` so a `)` or `;` inside a quoted function argument does not close the function node early. Added for formula args in 0.85.0 (#12444), it also swallowed `{{` — so `"{{step_4['result']}}"` re-parsed as raw text, and one unpaired quote earlier in a value killed every later chip in that field (GIT-1752). References are now emitted from inside the accumulation loop, and the string state is recomputed across the reference's interior. That recomputation is not optional: `concat("{{a"}}; lower(x))` puts the string-closing quote *inside* the reference, and skipping it leaves the following `;` inside the string and corrupts the value on re-save. +- **The builder's tokenizer and the runtime's are different code with different rules.** The engine uses `extractMustacheTokens` (`core/utils/src/lib/mustache-utils.ts`), which is brace-depth aware and completely quote-blind; the editor scans to the first `}}` and does track quotes. They agree on everything the UI can produce today, but a change to either is not automatically a change to the other — display and resolution can drift. +- **Three round-trip corruptions in that loop are known and unfixed**: a lone apostrophe in an unquoted formula argument, a newline inside function arguments, and an escaped backslash before a closing quote. They predate GIT-1752 and fail identically on older commits — don't treat them as a new regression when a round-trip test surfaces one. - Backward-compat hooks: `argCompatibility.defaultArgs` (fill missing trailing args from a default) and `deprecated: { replacement, removeAfter }` (strikethrough badge, still resolves at runtime). Never hard-remove a function; format bumps are handled by the `v\d+` wrapper (add `evaluateV2`, dispatch on captured version). ### Key files @@ -31,6 +34,6 @@ Entry point: `formulaEvaluator`, exported from `packages/core/formula/src/lib/fo - `packages/web/src/app/builder/piece-properties/text-input-with-mentions/extensions/` — the three inline atom badge nodes plus the `/` slash extension. - `packages/web/src/app/builder/piece-properties/text-input-with-mentions/components/` — function search and hover popovers. - `packages/core/shared/test/formula/` — evaluator, type-checker, and serializer round-trip tests. -- `packages/web/test/app/builder/piece-properties/text-input-with-mentions/` — serializer resilience tests (unclosed `{{`). +- `packages/web/test/app/builder/piece-properties/text-input-with-mentions/` — serializer resilience and round-trip tests (unclosed `{{`, quoted references, quoted function args). Paths verified 2026-07-17. An earlier version pointed at `packages/core/shared/src/lib/formula/`; it moved to its own package at `packages/core/formula/src/lib/` (`@activepieces/core-formula`). diff --git a/brain/knowledge/platform-editions-ee/ee-projects-rbac.md b/brain/knowledge/platform-editions-ee/ee-projects-rbac.md index 2c1c7f13daab..08f3eaf5f2b0 100644 --- a/brain/knowledge/platform-editions-ee/ee-projects-rbac.md +++ b/brain/knowledge/platform-editions-ee/ee-projects-rbac.md @@ -8,8 +8,8 @@ The EE Projects module adds team collaboration, role-based access control, git-b ### Members & roles - **ProjectMember** entity: `(projectId, userId, projectRoleId, platformId)`, unique on (projectId, userId, platformId). Service: `upsert`, `list`, `getRole` (returns ADMIN if owner/platform admin), `update`, `delete`, `getIdsOfProjects`. -- **ProjectRole**: named permission set, platform-scoped, `type` DEFAULT/CUSTOM. Built-in: **ADMIN** (all 26 permissions), **EDITOR** (read + write flows/folders/tables, update flow status), **VIEWER** (read-only). Custom roles behind `customRolesEnabled`. -- **Permission**: one of 26 granular capabilities (`READ_FLOW`, `WRITE_CONNECTION`, etc.). +- **ProjectRole**: named permission set, platform-scoped, `type` DEFAULT/CUSTOM. Built-in: **ADMIN** (every permission), **EDITOR** (read + write flows/folders/tables, update flow status), **VIEWER** (read-only). Custom roles behind `customRolesEnabled`. +- **Permission**: one granular capability (`READ_FLOW`, `WRITE_CONNECTION`, etc.), almost all of them READ/WRITE pairs per feature area. ### RBAC enforcement Yes, RBAC is a middleware layer. `rbacMiddleware` is registered once as a Fastify `preHandler` in `app.ts`, so every route passes through it. It resolves the route's project + permission and delegates to `rbacService`, which routes by principal type: **USER** goes to the member's role permission check; **ENGINE** checks `principal.projectId === requestedProjectId`; **SERVICE** checks `project.platformId === principal.platform.id`. UNKNOWN, WORKER and ONBOARDING are rejected outright. @@ -23,6 +23,7 @@ Note it lives under `ee/authentication/`, not `ee/projects/`, which is where mos - **Git Sync**: SSH repo URL + branch + folder path; push exports published flows/tables, pull imports as a release source; individual-item push supported. ### Gotchas +- **A new `Permission` needs a row in the role dialog, or custom roles can never grant it.** The toggle list is a hardcoded array, `initialPermissions` in `packages/web/src/app/routes/platform/security/project-role/project-role-dialog.tsx`, and the dialog is a plain `.map()` over it. Default-role grants are hardcoded separately in `access-control-list.ts`, so a permission added there but not here is invisible: ADMIN/EDITOR/VIEWER have it, custom roles cannot be given it, and the feature's tab just never appears for those members. This has already shipped twice — Variables + Knowledge Base (GIT-1751), then Agents. Nothing catches the drift: CI neither typechecks nor unit-tests `web`, so add the row in the same PR as the enum entry. - **Piece filtering** now via **piece sets** — `project.pieceSetId` (nullable FK, SET NULL). When `managePiecesEnabled`, new EE projects get the Default set on create; unassigned resolves to Default at filter time. This supersedes the legacy project-plan allow/block list. - **Worker routing**: `workerGroupId` (bare label, `^[a-z0-9_-]+$`) gated by `workerGroupsEnabled`. When set, the project's `EXECUTE_FLOW`/`EXECUTE_WEBHOOK` jobs route to `project-