brain/knowledge/engineering/ci-pr-review-hygiene.md
The CI gates that shape how a PR is reviewed, as opposed to whether it builds. Lives in .github/workflows/.
We open PRs as drafts so no human reviewer is auto-assigned until "Ready for review". Greptile's Review draft pull requests setting is enabled, so its first pass lands on draft open with no CI glue — first-pass AI review while it is still a draft, human review after. Unlike a once-per-PR CI nudge, the native setting also re-reviews as commits land on the draft.
pr-size.yml + tools/scripts/pr-size-check.ts count meaningful lines (additions + deletions, minus lockfiles, i18n/translation.json, locales/**, snapshots, dist) per area and fail when a gated area is over budget: engine+worker+execution combined 300, core/shared 250, server/api 600, packages/web 1200. packages/pieces and everything unmatched are measured but exempt — a line count can't tell a cohesive new piece from a codemod, and pieces are self-contained with low blast radius. Bypass with the large-pr-ok label or a revert: title. Budgets were calibrated from the distribution of recently merged PRs.
The diff comes from local git diff --numstat, not the /files API, so it is immune to GitHub's 3,000-file response cap — a mega-PR cannot under-count its way past the gate.
Which team gets asked to review comes entirely from .github/CODEOWNERS — there is no bot, no dependabot/renovate config, and no workflow that requests reviewers. @activepieces/core is the catch-all owner; @activepieces/pieces owns /packages/pieces/; @activepieces/platform owns the execution path (/packages/server/engine/, /packages/server/worker/, /packages/core/execution/). /bun.lock and /brain/ are listed with an empty owner column, which releases them from the catch-all — a PR touching only those needs no code-owner approval. Each team uses GitHub round-robin assignment, so one human per team per PR.
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.
git stash to prove a new test fails without its fix. Use git checkout <base> -- <file> instead. git stash push -- <path> on a path with no uncommitted changes saves nothing and creates no entry, so a following git stash pop silently pops whoever's stash is at stash@{0} instead. This repo carries long-lived stashes from other branches, so the pop conflicts, is kept, and still writes that stash's untracked files into the working tree, which then look like your own new files. It has happened at least twice, and stash@{1} is literally named "recovered: AGENTS.md agent-skills section (accidentally popped by claude)". Reverting one committed file to its base version and running the test there is the same proof with no shared state: git checkout <merge-base> -- <file>, run, then git checkout HEAD -- <file>. If a stash pop does go wrong, the entry survives the conflict, so the recovery is to delete the stray untracked files after confirming they belong to it with git stash show --include-untracked --name-only stash@{0}.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, a pieces-metadata-only PR, and 3 runs straight on #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 allowLists, 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.main's pending drift into your PR — run them, then keep only your own lines. npm run i18n:extract reorders all of en/translation.json and rewrites nine locale files (130 moved lines for six new keys), and bun install after a version bump writes back every community-piece version that was bumped without a lockfile sync (103 lines for four intended bumps). Both diffs are indistinguishable from real work in review, and both bury the change you actually made. Revert the file and hand-apply your own entries instead — then prove parity by running the generator into a scratch copy and diffing just your keys against it, so you keep byte-identical output without the churn. Provider setup markdown in features/agents/ai-providers.ts is extracted as translation keys in source order, so new entries go beside their neighbours in SUPPORTED_AI_PROVIDERS, not at the end..env.dev is TRACKED, so the .env* line in .gitignore does not protect it — secrets put there get committed. .gitignore line 82 is .env*, which reads as blanket protection for every env file, but gitignore has no effect on a path already in the index, and both .env.dev and .env.example are committed on main. git check-ignore .env.dev returns nothing, which is the tell. So an SMTP password or API key dropped into .env.dev shows up in git status as a normal modification and rides the next git add -A. Put local secrets under dev/ instead — that whole directory is genuinely ignored (line 27) — and reach for git check-ignore -v <path> before writing a credential anywhere, rather than trusting the pattern.* 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), and so did a single-page docs PR (#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.core request on a pieces PR is not always the lockfile — check for a second root file. #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.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.PR size is added as a required status check for main in branch protection. Until then it is visible but advisory.secrets.CROWDIN_PRS, not GITHUB_TOKEN. Despite the name, that PAT is this repo's open-a-PR-as-a-bot token: crowdin-pr-merger.yml, reusable-finalize-translations-pr.yml and — the tell — release-self-hosted.yml, which has nothing to do with Crowdin and uses it for both actions/checkout's token: and gh pr create's GH_TOKEN. Those jobs declare only permissions: contents: read, because the PAT does the pushing and the PR-opening; raising GITHUB_TOKEN to contents: write / pull-requests: write instead is treating the symptom, since Allow GitHub Actions to create and approve pull requests is evidently off for the org (not readable without admin:org). The failure mode is nasty because it is half-done and unattended: the branch pushes fine and only pulls.create fails, leaving an orphan auto/* branch every scheduled run. Copy release-self-hosted.yml, and have the job delete its own branch on failure so a bad week retries clean instead of accumulating.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.validate-publishable-packages compares against npm, not against main, so touching a published piece without bumping it fails CI on its own. The error is [packagePrePublishValidation] package version not incremented, path=packages/pieces/community/<piece>, version=X. Editing any file in a published package is enough — a one-line change to the AI piece's model factory tripped it while @activepieces/piece-ai sat at 0.9.0 on npm. Check with curl -s https://registry.npmjs.org/@activepieces/piece-<name> | jq -r ."dist-tags".latest, and follow the piece's own history for the size of the bump: capability additions have gone minor, fixes patch. The version lives in two files — package.json and bun.lock, which records each workspace's version — so bump then bun install, or the lockfile check fails instead. Distinct from the merge-drift trap below: this one fires before any merge, and only for packages that are actually published.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.NODE_OPTIONS=--max-old-space-size=8192. npx eslint packages/server/api/test/.../<file>.test.ts on one file died with FATAL ERROR: Reached heap limit after ~23s at 2GB, because the type-aware config loads the whole packages/server/api program regardless of how few files you name. It reads as a broken lint setup, not as a memory ceiling. The same run with an 8GB heap finishes and reports normally.bun install on a recent bun adds "configVersion": 0 to bun.lock, which is not on main. It rides along in any commit that touches the lockfile and reads as an unrelated change; drop the line and re-run bun install --frozen-lockfile to confirm the lockfile is still consistent without it.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 after the six-providers PR landed: core-piece-types and pieces-framework auto-merged at 0.5.0 / 0.37.0 and both needed a further bump. Only a conflicting version (like core/shared 0.140.0 vs 0.141.0) forces you to think; the clean ones are the dangerous ones. After any merge, re-check every package you bumped against git show origin/main:<pkg>/package.json. The reverse also happens: when review makes you delete code, the bump it justified can become dead — after acting on review, git diff origin/main...HEAD -- <pkg>/src and drop the bump if it is empty. On #15001 two packages ended up byte-identical to main while still carrying a bump, which is noise at best and a version collision at worst.@activepieces/shared re-exports from @activepieces/core-execution, so a partial rebuild produces phantom "has no exported member" errors in unrelated files. Rebuilding core/shared against a stale core/execution dist drops those re-exports, and the API typecheck then fails in ee/agent/* on symbols like GetPersonalizationConfigRequest — which live in core/execution/src/lib/workers/worker-contract.ts, not in shared at all. It reads exactly like a bad merge. The dependency order that actually works is core/utils → core/piece-types → core/formula → core/execution → core/shared → server/utils → pieces/framework → core/ai-providers; skipping a link silently poisons everything downstream of it. The same staleness makes an editor report missing enum members that exist in the source.origin/main. A PR's diff is computed against the merge-base, so git checkout origin/main -- <file> does not "revert" the file — it imports every change main made to it since the fork and attributes them to you. Dropping one web file from #15001 that way would have silently added 82 insertions / 44 deletions of somebody else's work. git checkout $(git merge-base origin/main HEAD) -- <file> makes it byte-identical to where the branch started, so it leaves the diff entirely and merges cleanly instead of conflicting. Verify with git diff --quiet $(git merge-base origin/main HEAD) -- <file> before committing, and read git status first — a bun.lock left dirty by an earlier bun install loves to ride along on a commit like this.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 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...<branch> 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.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.setup-environment.yml also triggers on closed. Both workflows fire on the same close event; Remove Environment tears the env down correctly (compose down, nginx, repo), then Setup Environment sees the preview label (labels survive merge) and re-provisions the whole thing minutes later — verified on #14832: remove finished 11:20, setup rebuilt it by 11:30. This is why merged PRs kept live zombie environments on the preview box. Both workflows are thin SSH wrappers; the real setup/remove logic lives in /root/environments on the preview server (secrets.PREVIEW_HOST), not in this repo. Fixed by dropping closed from setup's trigger list.stop() skips docker compose down when repos/<subdomain>/docker-compose.yml doesn't exist, so an env whose repo folder was deleted first leaves containers running forever — re-running remove is a no-op for them. Clean those manually via compose labels: docker ps -aq --filter "label=com.docker.compose.project=<subdomain>" (same filter works for docker volume ls). When auditing envs against PR state: read the real branch from the clone's HEAD (git -C repos/<subdomain> symbolic-ref --short HEAD) since subdomains flatten / to -; a clone sitting on main means the branch was deleted after merge; and an env with no PR at all is a manual workflow_dispatch preview — don't auto-delete those (bulk cleanup 2026-08-20 removed 27 closed-PR envs, reclaimed 32.5GB).passwordless-authn.test.ts lives under test/integration/ce/authentication/ on main, and a branch may carry its own copy elsewhere — a behaviour change to requestCode or signUp has to update every copy. This bites hardest after rebuilding a branch onto a different base, which resurrects files the old base had moved: the edit list from the first attempt is then silently incomplete, and because api unit tests do not gate CI (below), the edition copy is the only thing that catches it. Grep the assertion (DOMAIN_NOT_ALLOWED, the fixture domain) across test/ rather than the filename.204 and kept passing after the guard it covered stopped running at all. Any silent-failure design has to be pinned on side effects — rows created, mail sent, spies called — because the response is by construction indistinguishable.vi.mock is hoisted above imports. The existing worker-group.service.test.ts reaches for await import(...) to load its subject after the mocks, which is unnecessary and, if you copy it to the top level of a file rather than inside a function, fails tsc -p tsconfig.spec.json with TS1378: Top-level 'await' expressions are only allowed when the 'module' option is set to …. Vitest itself runs it happily and lint says nothing, so the only thing that catches it is a typecheck nobody gates on. A plain import { thing } from '…' alongside the vi.mock calls works and typechecks.packages/server/api/test/unit/ never runs in CI. ci.yml runs exactly two test commands: turbo run test filtered to engine/shared/sandbox/ai-providers/pieces-framework/web, and turbo run test-ce test-ee test-cloud check-migrations --filter=api. The api package has a test-unit script (vitest run test/unit), but no workflow invokes it and the root test-unit filter list does not include api — so the 10+ files already sitting in test/unit/** are dead weight, and a new one passes review while protecting nothing. packages/core/execution is in the same position. Until the wiring changes, put api coverage that must actually gate merges in test/integration/ce|ee|cloud, and if you do add a unit test, say in the PR that you ran it locally and paste the result.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.keep-open first. close-external-prs.yml triggers on pull_request_target [opened, reopened], so every reopen re-runs the same comment-then-close step; its if exempts OWNER/MEMBER/COLLABORATOR, bots, and the keep-open label, and nothing else. A docs PR from an outside contributor (#15031) was reopened 13 times over two days and closed 13 times within seconds of each, until a member labelled it keep-open and reopened it once. The same job also runs a nightly actions/stale pass that closes any PR idle 60 days. The lasting fix for a change worth keeping is to re-open it from a branch owned by someone with write access — author association, not the diff, is what the gate reads.license/cla keys off the commit author email, so re-opening someone else's branch under your own name does not clear it. CLA-assistant walks every commit in the PR rather than the PR author, and an author email that matches no GitHub account can never be matched to a signature — the 47 commits carried over onto #15092 were authored as [email protected], a local hostname, so the check sat at not_signed on a PR opened by a member. It is not in the main ruleset's required-checks list, but it is red on the page and a reviewer reads that as unmergeable. Either the original author signs through the PR link, or the commits get re-authored to an email tied to their GitHub account before you open it.brain/ → brain/knowledge/ move cannot edit a brain page in place — GitHub will call the PR conflicting even when git merge is clean locally. Git follows the rename and merges the modification into the new path; GitHub's mergeability check does not, so it reports modify/delete on the old path and the PR goes dirty. Local git merge-tree --write-tree exits 0 and hides the problem; reproduce what GitHub sees with git merge -X no-renames origin/main. Fix: merge origin/main into the branch first, which lands the edit at the new path, then push.breaking-change-check couples the docs entry to the label in BOTH directions, so back-documenting an already-shipped change drags the label onto a docs-only PR. R3 in tools/scripts/breaking-change-check.ts fails a PR that adds a #### entry to docs/install/reference/breaking-changes.mdx without ⛓️💥 breaking-change, exactly as it fails the label without an entry — and the template answer has to agree too, so "yes" must be ticked on a PR that changes no code. It reads the added lines of that one file from git diff origin/<base>...HEAD, and hasBreakingEntry wants a #### heading plus a non-heading body line, so a heading alone, a ---, or a version bump does not count. Two consequences: the label then collides with skip-changelog in release-drafter (pick one deliberately — the feature's own PR usually already carried the changelog entry), and an entry appended to a released section still trips it, since the check never looks at which heading the lines landed under.## Unreleased over at release time, and the docs site is unversioned — so a breaking-changes entry has to name its own version. No workflow or script writes to docs/install/reference/breaking-changes.mdx (breaking-change-check.ts only reads it), and git log -S"## 0.88" on the file comes back empty: the heading has not moved since 0.87.0, so entries for work that shipped months ago still sit under "Unreleased" (PM2 removal in 0.88.2, cache pre-warm gate and workspace naming in 0.89.0, …). docs/docs.json has no versioning either, so there is one live page for every self-hoster whatever version they run, published on merge rather than on release — the version heading is the only thing telling a reader whether a change is already in their build. So before adding an entry, run git tag --contains <commit> on the change it describes and file it under the release that actually shipped it; only genuinely unshipped work belongs under "Unreleased". What points self-hosters at the page in the first place is release-drafter.yml, which appends a "review the Breaking Changes page" line to every release body and groups ⛓️💥 breaking-change PRs under their own heading — which also means a docs-only PR back-documenting an old change shows up in the next release's breaking-change list.const sitting above the file's exported symbol (P2 on #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.