[pull] main from activepieces:main

brain/knowledge/decisions/000027-ai-provider-resolution-takes-a-required-scope-and-splits-reads-by-trust-level.md

@@ -0,0 +1,21 @@
+---
+status: accepted
+---
+
+# AI provider resolution takes a required scope, and its reads split by trust level
+
+## Decision
+
+Every AI-provider resolver takes a **required** `ProviderScope` — `{ type: 'project', projectId } | { type: 'platform' }` — with no default and no optional `projectId`. The read endpoints split by who is asking: `GET /v1/ai-providers?projectId=` and `GET /v1/ai-providers/:provider/models?projectId=` are project-scoped (`securityAccess.project([USER, ENGINE], undefined, QUERY)`) and always apply the resolved key's model allow-list, while `GET /v1/ai-providers/configs` and `GET /v1/ai-providers/configs/:id/models` are `platformAdminOnly`, address an exact row, and return the unfiltered model list. The project list is deduped to one entry per provider and carries only `{ provider, name, enabledForChat }`.
+
+## Context
+
+Per-key project and model scoping arrived with the multi-key redesign (see [providers-redesign-before-routing](providers-redesign-before-routing.md)). The first cut made `projectId` optional on `resolveEligibleRow` because USER principals carry no project, and treated its absence as "every key on the platform is eligible". Over four review rounds the same finding was filed at four different locations — the MCP and chat paths, the agent piece and knowledge-base tool handlers, the chat/agent model picker, and the `configId` lookup — because each new caller that omitted the argument silently got platform-wide access. Separately, one `/models` route served the engine, ordinary project users and the platform admin console, so each fix aimed at one consumer opened a hole for another: addressing a key by name resolved the wrong row under multi-key, and adding a `configId` query param let any member reach a configuration excluded from their project.
+
+## Why
+
+A fail-open default in a credential resolver cannot be fixed by patching call sites, because the defect is the default and every future caller re-creates it. Making the argument required converts an omission into a compile error, and spelling `{ type: 'platform' }` turns the wide case into a claim a reviewer can check rather than an accident — only three consumers legitimately need it (the tool-search embedder, chat memory extraction, and the managed ACTIVEPIECES singleton). The route split follows the same principle at the HTTP boundary: `securityAccess` is per-route, so one route serving two authorization levels forces hand-rolled authz inside the handler, which is exactly the branch that leaked. Two routes let each state its own security config honestly.
+
+## Consequences
+
+`GET /v1/ai-providers` narrows: it no longer returns `id`, `config`, `modelIds` or `projectIds`, so a project caller can no longer see other projects' identifiers, and any new consumer wanting a full configuration must be a platform admin going through `/configs`. The AI piece's provider dropdown keeps working unchanged (it authenticates as ENGINE, which supplies its own project) but now sees one entry per provider instead of one per key, and its `AIProviderWithoutSensitiveData[]` annotation is wider than the real response. A project with no eligible key 404s rather than falling back to a platform key — scoping only ever narrows. Model routing, when revived, must keep addressing keys by row id and pass a scope like everyone else.

brain/knowledge/decisions/000030-a-step-may-pin-an-ai-provider-key-and-omitting-one-can-only-narrow.md

@@ -0,0 +1,70 @@
+---
+icon: 📌
+status: accepted
+---
+
+# A step may pin an AI provider key, and omitting one can only narrow
+
+## Decision
+
+An AI step and an agent may name the exact `ai_provider` row they run on. The picker is **one flat
+list of keys**, not a provider list plus a key list: each option is a key, labelled
+`Anthropic: key 1` when its provider holds more than one and plainly `Anthropic` when it holds one.
+The AI piece's `provider` prop therefore stores `{ provider, configId }` rather than a bare enum, and
+an agent stores `AgentConfig.providerConfigId` beside its `provider`. Every read routes
+through one resolver, `resolveRowForScope({ platformId, provider, scope, configId })` — with no
+`configId` it falls back to the deterministic ranking (`selected` > `except` > `all`, newest first);
+with one it serves that row only after checking it belongs to the platform, matches the named
+provider, and passes `rowAllowsScope` for the caller's project. Key names are unique per
+`(platformId, provider)`, checked in the service.
+
+## Context
+
+The multi-key redesign gave a platform several keys per provider, but `listForProject` deduped them to
+one entry and a step stored only an `AIProviderName`, so the builder showed a single row labelled with
+the winning key's display name. An admin who configured "Anthropic key 1" and "Anthropic key 2" saw
+one of them and reasonably read the other as lost; two keys both scoped to *all* projects meant the
+newer one won everywhere and the older never executed.
+
+## Why
+
+Scoping alone cannot express "this step, that key" — it can only express "this project, that key",
+and a project routinely wants a cheap key for one step and a production key for another.
+
+The obvious risk was reopening the fail-open hole that decision
+[000027](000027-ai-provider-resolution-takes-a-required-scope-and-splits-reads-by-trust-level.md)
+closed, since `configId` is optional by nature. It does not, and the asymmetry is the point: a
+forgotten `configId` degrades to the deterministic winner, which is by construction already eligible
+for that scope, so an omission narrows-or-equals and never widens. `scope` stays required and is
+still what authorizes the row.
+
+The first cut used two dropdowns — Provider, then an optional Configuration — to keep `provider` a
+bare enum, since a flat list makes the stored value stop being a provider name and every action that
+branches on it (web search, image capability, `getEffectiveProviderAndModel`) has to read it back
+out. That was rejected on use: an admin who configures three keys expects to *see* three lines, and a
+vendor row that silently resolves one of them is the confusion the redesign set out to fix. The
+parsing risk is contained by making the value an object rather than a composite string and by giving
+the piece exactly one accessor, `aiProviderSelection.resolveOrThrow`, which also maps a legacy plain
+string to `{ provider, configId: undefined }` so flows saved before the change keep resolving
+automatically. A per-key *route* was still rejected: `/:provider/config?configId=` already carries
+both, and a second route would split the trust-level check that decision 000027 consolidated.
+
+Names are constrained because a picker showing two rows called "Anthropic key" is unusable. The check
+lives in the service rather than a unique index: an admin racing themselves is not a real threat, and
+the index costs a migration and a Postgres error to map back to a form message. Duplicate
+*credentials* stay legal — one secret with two allow-lists is a supported setup, and the random-IV
+encryption makes duplicates undetectable anyway.
+
+## Consequences
+
+`ProjectAIProvider` now carries `keys: [{ id, name }]` for the project-facing list, and its `name` is
+the vendor label (`aiProviders[provider].name`) rather than a key's display name; the list itself
+stays deduped, so `ap_list_ai_models` and the agent selector are unaffected. `GetProviderConfigResponse`
+returns `configId`, which the chat worker echoes back on piece and knowledge-base tool calls, so every
+call in one turn runs on the same key instead of re-ranking mid-run.
+
+A pinned key becomes a hard dependency: delete it, or scope its project away, and the step fails at run
+time rather than sliding to another key. That is the intended trade for an explicit choice, but it is
+the reason the first cut avoided pinning. Because the flat list always carries a `configId`, every
+step created after this change is pinned — automatic resolution now only applies to steps saved
+before it, and to server-side consumers (chat, embedder, memory extraction) that name no key.

brain/knowledge/decisions/providers-redesign-before-routing.md

@@ -0,0 +1,21 @@
+---
+status: accepted
+---
+
+# Providers redesign ships before model routing
+
+## Decision
+
+The 2026-08 replan of "AI Providers — One Experience, Any Provider" swaps milestone order: the AI providers page redesign (multi-key providers, per-key model allow-lists, per-key project scoping, "AI Center" page) is milestone 2; model routing moves to milestone 5. Routing PRs #14563 (backend) and #14587 (setup UI) stay parked open to revive later.
+
+## Context
+
+Routing was originally milestone 2 with its engine PR already open. Meanwhile the providers-page prototype (`proto-ai-providers-ui-v2`) converged on a data model where one platform holds multiple keys per provider, each with its own model and project scope.
+
+## Why
+
+The providers foundation must land first: routing's slot shape `{provider, modelId}` cannot address a specific key once multiple keys per provider exist, so routing built now would need rework. Routing's own scope also grew (metadata catalog, capability matching, custom tiers) beyond a single milestone slot.
+
+## Consequences
+
+Runtime key resolution is deterministic without a priority field: most specific project scope wins (selected > except > all), newest `created` breaks ties. Model routing, when revived, must address keys (rows), not provider names. Model facts (cost/context/speed) stay out of scope until a real catalog exists — pickers are names-only.

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

@@ -22,6 +22,7 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def
 ## Gotchas
 - **A bare `*` in CODEOWNERS matches every file at every depth, so the catch-all owner is dragged into PRs that have nothing to do with them.** Unlike `docs/*` (direct children only), `*` is fully recursive, and last-match-wins means only an explicit later rule can release a path. A lockfile-only PR requested `core` ([#14629](https://github.com/activepieces/activepieces/pull/14629)), and so did a single-page docs PR ([#14422](https://github.com/activepieces/activepieces/pull/14422), one file under `brain/`). The release valve is a **path listed with no owner after the `*` line**, which GitHub reads as owned-by-nobody; CODEOWNERS has no `!negation` syntax and no brace expansion — `packages/**/{A,B}.md` parses clean and matches a file literally named `{A,B}.md`. Verify any edit with `gh api repos/activepieces/activepieces/codeowners/errors` — an invalid line is silently *skipped*, which quietly restores the catch-all owner instead of failing loudly.
 - **A spurious `core` request on a pieces PR is not always the lockfile — check for a second root file.** [#14558](https://github.com/activepieces/activepieces/pull/14558) looked like the lockfile case but its non-pieces files were `bun.lock` *and* `tsconfig.base.json`; the `core` request landed 6s after the commit that touched the tsconfig, not after the pieces push. Per-piece `paths` mappings generated into root `tsconfig.base.json` mean a pieces change can still reach a core-owned file, and no CODEOWNERS pattern can fix that — the file holds real compiler options and CODEOWNERS has no sub-file granularity.
+- **Greptile's Confidence Score prose is cumulative — a low score is not evidence of a live problem.** It edits one summary comment in place, and its "Files Needing Attention" list keeps naming findings that are already resolved and outdated: #14825 sat at 2/5 citing three files, two of which were a closed P1 and a duplicate view of the third. Read the *unresolved* review threads (`reviewThreads(first:60) { isResolved isOutdated }` over GraphQL — the REST comments endpoint carries no resolution state) and judge from those; re-trigger the review to refresh the score. It also re-raises the same class of finding each round with a new comment id, so a fix on one thread does not silence its sibling.
 - **A red check does not block a merge.** The gate only prevents merges once `PR size` is added as a **required status check** for `main` in branch protection. Until then it is visible but advisory.
 - **Workflow actions are pinned to major-version tags, not SHAs** (`actions/checkout@v5`, `oven-sh/setup-bun@v2`). The only SHA pins live in the CodeQL security workflow. Reviewers — human and AI — regularly suggest SHA-pinning a single new workflow; decline it. Moving to SHA pinning is a repo-wide policy call, and a half-pinned `.github/` is worse than a consistent one.
 - **`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`.

brain/knowledge/engineering/server-module-anatomy.md

@@ -135,7 +135,7 @@ Verify with `npm run lint-dev` and `npm run test-api`.
 - **The migration generator emits the wrong interface.** Every generated file must be patched from `MigrationInterface` to this repo's `Migration`, or CI rejects it. Never hand-write the SQL instead — generate from the entity diff, then patch.
   - **Exception: the generator diffs against *your* database, so a surviving table of the same name produces an `ALTER`, not a `CREATE`.** Adding `AgentEntity` (table `agent`) emitted a mutation of the dead 2025 `agent` table — `DROP COLUMN systemPrompt`, then `ADD "iconKey" character varying NOT NULL` with no default, which fails on any table that has rows — and left `agent_run` untouched. When you are deliberately replacing an orphaned table, hand-write `DROP TABLE IF EXISTS … CASCADE` + `CREATE TABLE`, and check `pg_constraint` for FKs pointing at it first.
 - **`npm run check-migrations` can pass while your database is untouched.** It sources `.env.tests` (not `.env.dev`) and pipes `migration:run` to `/dev/null`, so it reported "No changes in database schema were found" against a database still holding the pre-migration table. Treat a green run as "the entity and *some* database agree", not as proof your migration executed. To actually verify, run `migration:run` against the dev DB with an explicit `AP_POSTGRES_HOST` and then inspect `information_schema.columns`, `pg_indexes` and `pg_constraint`.
-- **Migration timestamps collide across unmerged branches.** `migrations` is keyed by class name, so two branches can both claim `1824000000000` and only conflict at merge. Before picking a timestamp, check the applied ledger (`select name from migrations order by id desc limit 5`) as well as the files on `main` — a timestamp can already be in use by a branch you cannot see.
+- **Migration timestamps collide across unmerged branches.** `migrations` is keyed by class name, so two branches can both claim `1824000000000` and only conflict at merge. Before picking a timestamp, check the applied ledger (`select name from migrations order by id desc limit 5`) as well as the files on `main` — a timestamp can already be in use by a branch you cannot see. When the collision does surface in a merge, renumber **yours** — the one on `main` is already applied in production and cannot move — which means renaming the file, the class, and the class's `name` field, then re-registering it after the merged one in `getMigrations()`. Whoever already ran the old name locally needs no DB surgery *provided* `up()` is idempotent (`IF NOT EXISTS` / `DROP … IF EXISTS` throughout): TypeORM sees an unapplied name and re-runs it as a no-op. Without that, they have to update the `migrations` ledger row by hand.
 - **PGlite has one connection, so `CONCURRENTLY` breaks it.** Guard on `system.get(AppSystemProp.DB_TYPE) === DatabaseType.PGLITE` and issue a plain `CREATE INDEX` on that branch. When you do use `CONCURRENTLY`, set `transaction = false` on the migration class — PostgreSQL requires it outside a transaction.
 - **`EntitySchema` supports partial-index `where`, but not expression columns.** For a partial index on a bare column (e.g. `ON file(platformId) WHERE projectId IS NULL`), pass `where: '"projectId" IS NULL'` alongside `columns: ['platformId']` — TypeORM 0.3.x's `EntitySchemaIndexOptions.where` is honored by the Postgres driver (`PostgresQueryRunner` line 2442: `${where ? "WHERE " + where : ""}`), so `synchronize` can stay on and `migration:generate` tracks the index correctly. Reserve `synchronize: false` for **expression indexes** — `columns` is `string[]` of bare column names with no expression syntax, so an index like `ON file(type, (metadata->>'flowId'))` (see `idx_file_sample_data_flow_id`) genuinely can't be expressed and needs the opt-out. Blindly using `synchronize: false` for every hand-written index (which I did once and got called on) leaves TypeORM blind to the index — future `migration:generate` won't drop it if you remove it from the entity, and drift can silently accumulate.
 - **`UpdateResult.affected` is `undefined` on PGlite — never branch on it.** TypeORM's Postgres driver sets `affected` from `raw.rowCount`, and `typeorm-pglite` returns PGlite's `Results` (`{ rows, fields, affectedRows }`) with no `rowCount`. So the compare-and-set idiom `if (result.affected === 0) return null` is *always false* on PGlite and every predicate in the `WHERE` becomes decorative — the guard silently passes. This is not test-only: `AP_DB_TYPE=PGLITE` is the documented one-line Docker install (`docs/install/options/docker.mdx`). It hit MCP OAuth (`mcpOAuthCodeService.consume`), where it made authorization codes replayable, unbound to their client and redirect_uri, and immune to expiry. Use `.returning('*')` and test `updateResult.raw` for emptiness instead — that works on both drivers. Confirmed against the pinned `@electric-sql/pglite` 0.3.14: a plain `UPDATE` answers `{ rows, fields, affectedRows }` with **`rowCount: undefined`**, while the same statement with `RETURNING *` fills `rows` correctly (0 on no match, 1 on match). Note PGlite *does* report `affectedRows` — it is only `rowCount`, the field TypeORM reads, that is missing, so "PGlite loses the count" is the wrong mental model. The remaining call sites were converted in 2026-08 (`ee/agent/agent-rpc-handlers.ts`, `ee/projects/platform-project-service.ts`); a `.affected` that only feeds a log line was left alone. **Integration tests here run on Postgres** (`.env.tests` points at a real server), so they cannot catch this class at all — run the suite with `AP_DB_TYPE=PGLITE` prefixed to exercise it, which works today and is how the fix was proven red-to-green. Prefer `.returning('id')` over `.returning('*')`: on a table like `agent_conversation` the star form hauls the whole `messages` jsonb back on every write, and a row only has to be counted, not read.