# Tuist Elixir Review

> Project-specific PR-review rules for the tuist/tuist Elixir codebases (server, cache, processor, xcode_processor, tuist_common, noora). Focuses on the things only this repo knows — authorization invariants, tenancy, write-only ClickHouse, Mimic placement, migration timestamptz, data-export updates, marketing changelog entries, and i18n.

- Skill: `tuist/tuist-elixir-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add tuist/tuist-elixir-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/tuist/tuist-elixir-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Marketing & Growth
- Author: tuist (https://skillmd.com/u/tuist)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/tuist/tuist-elixir-review

---


# Tuist Elixir Review

This skill is intentionally narrow. **Generic Elixir style, naming, pipe
chains, formatting, nesting depth, and `String.to_atom/1`-style hygiene
are already covered by `mix format` and `credo` in CI — do not flag
those.** Focus on the rules below; they catch real bugs.

For each finding, cite `path:line` (or `Module.function/arity`) and
quote the relevant snippet.

Only report findings whose cited snippet is present in the PR diff. If
the concern comes from unchanged context, do not emit a finding, do not
mention it as a note, and do not create a "findings outside this PR's
diff" section. If every possible concern is outside the diff, return no
findings.

Do not infer violations from nearby lines. A Mimic finding requires the
exact token `Mimic.copy(` on the cited changed line. A migration
timestamp finding requires the cited changed line to contain
`timestamps()` without `type: :timestamptz` or a timestamp column without
`:timestamptz`.

---

## 1. Authorization — `lib/tuist/authorization.ex` + `AuthorizationPlug`

The policy DSL is `LetMe.Policy`. Categories are declared via `object
:foo do ... end` blocks. The API plug at
`server/lib/tuist_web/plugs/api/authorization/authorization_plug.ex`
hard-codes which categories are project-scoped:

```elixir
@project_categories [:run, :bundle, :cache, :preview, :test, :build, :automation_alert]
```

### Flag

- **A new `object :foo do` block (project-scoped resource) without `:foo` being added to `@project_categories`** in `authorization_plug.ex`. The route guard will silently miss the new category. **Severity: high.**
- **An `action` that uses `[:authenticated_as_project, ...]` but omits `:projects_match`.** This lets an authenticated project act on *another* project's resource. **Severity: critical.** The canonical correct form is `allow([:authenticated_as_project, :projects_match])`.
- **`:public_project` or `[:authenticated_as_user, :ops_access]` allowed for `:create`, `:update`, or `:delete` actions.** These flags are intended for read-only paths.
- **A new `action :read | :create | :update | :delete` that doesn't cover all three subject kinds** (`:authenticated_as_user`, `:authenticated_as_project`, `:authenticated_as_account` with a `scopes_permit:` check) **without an inline `desc(...)` explaining the omission.** Missing one is usually a bug; an explicit `desc` is the documented escape hatch.
- **An account-token `allow` without a `scopes_permit:` check** (e.g. bare `[:authenticated_as_account]`). Account tokens must always be scope-gated.

### Do not flag

- Existing `object`/`action` blocks unchanged by the diff.
- Reordering of `allow(...)` lines within an action.
- `/ops` LiveView routes. They are not API `AuthorizationPlug`
  categories and do not belong in `@project_categories`.

---

## 2. Tenancy — bare `Repo.get` on multi-tenant schemas

Tenant-owned schemas include at least: `Bundle`, `Run`, `Cache`,
`Preview`, `CommandEvent`, `Build`, `Test`, `Project`,
`AutomationAlert`. They all carry a `project_id` or `account_id`.

### Flag

- **`Tuist.Repo.get(Schema, id)` / `Repo.one(from(s in Schema, where: s.id == ^id))` without a `project_id` / `account_id` constraint** for any of the schemas above, when the call is inside a controller, plug, LiveView, channel, MCP handler, or worker that already has the project/account in scope. This is a tenant leak (an attacker who guesses a UUID gets cross-tenant data). **Severity: high.**
- **A new context function that takes an `id` and forwards it to `Repo.get` without also taking the project/account.**

### Do not flag

- Internal background jobs that intentionally operate across tenants (look for an explicit `# admin / cross-tenant: ...` comment or a function name like `*_for_all/_global/_admin`).
- Reads from non-tenant tables (`User`, `Account`, `Organization`, `Subscription`, etc.).
- **Webhook handlers operating on a row that was already cryptographically selected upstream.** When `lib/tuist_web/plugs/webhook_plug.ex` resolves a per-row HMAC secret (e.g. `GitHubController.resolve_webhook_secret/1` matches a `GitHubAppInstallation` row whose `webhook_secret` HMACs the raw body, then stashes the row on `conn.assigns[:github_installation]`), downstream handlers reading that assign do not need a separate `installation.account_id == expected_account_id` check. There is no separate "expected account" — webhooks land on a global `/webhooks/<provider>` URL, and the row *is* the tenant context, selected by a per-row cryptographic capability. A redundant `account_id` equality check after `valid_signature?/4` would compare the row's value to itself; it adds dead code, not a defense layer. If the cryptographic check fails, the request 403's before the handler ever runs.
- **Internal dispatch paths whose inputs come from a query already scoped by tenant.** When a function receives a struct produced by an upstream context function that already filters by `project_id` / `account_id` (e.g. `FlakyTestsMonitor.evaluate/1` → `AlertEvaluationWorker` → `ActionExecutor`), do not flag the downstream call as needing its own scoping check. Trace the input chain before flagging; only flag when the input is user-controllable (URL param, body field, header).

---

## 3. ClickHouse `IngestRepo` is write-only

There are **two** ClickHouse repos in this codebase, and they are not
interchangeable. Be precise about which one a call uses before flagging.

- **`Tuist.IngestRepo`** — write-only ingest path. Application code must
  not read from it; reads happen out of band.
- **`Tuist.ClickHouseRepo`** — the read-only ClickHouse repo (declared
  with `read_only: true` in `server/lib/tuist/clickhouse_repo.ex`).

### Flag (Severity: high)

- Any call to `Tuist.IngestRepo.all/1`, `Tuist.IngestRepo.get/2`,
  `Tuist.IngestRepo.get_by/2`, `Tuist.IngestRepo.one/1`,
  `Tuist.IngestRepo.exists?/1`, or `Tuist.IngestRepo.aggregate/3`.
- Any `from(... ) |> Tuist.IngestRepo.<read fn>`.

The fix is almost always to read through `Tuist.ClickHouseRepo` (for
ClickHouse-only data) or `Tuist.Repo` (PostgreSQL).

### Do not flag

- `Tuist.ClickHouseRepo.all/1`, `Tuist.ClickHouseRepo.one/1`,
  `Tuist.ClickHouseRepo.aggregate/3`, or any other read through
  `ClickHouseRepo`. That repo exists specifically for application reads.
  Do **not** confuse it with `IngestRepo`.
- Writes via `Tuist.IngestRepo.insert/2` / `insert_all/2,3` — those are
  the intended use.

---

## 4. Test setup — Mimic copies belong in `test_helper.exs`

`server/test/test_helper.exs` is the single place where
`Mimic.copy(Module)` is called. Per-test-file `Mimic.copy/1` calls leak
state across tests and are an explicit anti-pattern in this repo.

### Flag

- A `Mimic.copy(...)` call inside any file under `server/test/` other
  than `test_helper.exs`. Suggest: move it to `test_helper.exs`.

### Do not flag

- `Mimic.expect/3`, `Mimic.stub/3`, `Mimic.reject/1` — those belong in tests.
- `Mimic.copy/1` calls in `test_helper.exs` itself.
- `import Mimic`, `use Mimic`, `setup :set_mimic_from_context`, aliases,
  or any other test setup line that does not contain `Mimic.copy(`.
- A test file that merely uses Mimic (`use Mimic`, `import Mimic`,
  `stub`, `expect`, `reject`) but does not contain the exact
  `Mimic.copy(` call in the diff.

---

## 5. Migrations — timestamps must be timezone-aware

In `server/priv/repo/migrations/` and `server/priv/ingest_repo/migrations/`:

### Flag (Severity: medium)

- A new column declared as `timestamps()` without
  `type: :utc_datetime_usec` *and* a corresponding migration column
  without `:timestamptz`. The `.credo.exs` rule says: migrations use
  `:timestamptz`, schemas (`lib/`) use `:utc_datetime`.
- `add :inserted_at, :naive_datetime` or `:datetime` without timezone in a migration. Should be `:timestamptz`.

### Do not flag

- `timestamps(type: :timestamptz)`.
- `def change do`, `create table(...)`, blank lines, comments, or any
  line that does not itself declare a timestamp type.
- `add :started_at, :timestamptz`, `add :finished_at, :timestamptz`,
  or any other explicit `:timestamptz` column.

---

## 6. `data-export.md` updates on schema changes

`server/data-export.md` documents every piece of customer data Tuist
stores, for GDPR Article 20 / CCPA exports. **It must be updated when
the diff includes** any of:

- A new migration adding a table
- A new migration adding a column that stores customer / user / project
  data (not internal bookkeeping)
- A new Ecto schema in `server/lib/tuist/**/*.ex` that maps to a
  customer-facing table
- New file storage paths in S3 (e.g. new keys under `bundles/`,
  `previews/`, `caches/`)

### Flag (Severity: medium)

- A diff that touches `server/priv/repo/migrations/*.exs` (other than
  pure index / constraint changes) or `server/priv/ingest_repo/migrations/*.exs`
  **without** also modifying `server/data-export.md`.

This is a compliance gap, not just a docs nit — call it out clearly.

---

## 7. i18n — currency symbols are not translatable

In marketing copy and pricing UI:

### Flag

- Currency amounts (`€`, `$`, `£`, `¥`, currency codes) wrapped inside
  `dgettext/2` or `gettext/1`. Symbols and amounts must remain identical
  across languages.
- Example anti-pattern: `dgettext("marketing", "0€ and up")`. The
  correct form keeps the currency literal outside the translation:
  `"0€ " <> dgettext("marketing", "and up")`.

### Do not flag

- Descriptive text *around* prices (e.g., the words "and up", "per
  unit", "billed annually") — those should be translated.

---

## 8. Translation files — `.po` is read-only for humans

### Flag (Severity: high)

- Any modification to `server/priv/gettext/**/*.po`. Only the `tuistit`
  bot may edit `.po` files; CI will fail otherwise.
- Use of `mix gettext.extract --merge`. Only the no-`--merge` form is
  allowed in PRs.

### Do not flag

- `.pot` (template) changes — those are produced by `mix gettext.extract`
  and are expected when adding new translatable strings.

---

## 9. N+1 queries — DB calls inside loops

A `Repo.*` / `ClickHouseRepo.*` / `IngestRepo.*` call inside `Enum.map`,
`Enum.each`, `Enum.flat_map`, `Enum.filter`, `Enum.reduce`, `for`, or
`Stream.*` is almost always an N+1. Each iteration is a separate round
trip; the chart-bucket loop or per-row preload that looked harmless on
toy data stalls real page loads.

**Actively search the diff for these patterns** before signing off
— don't just react to obvious cases:

- `Enum.map(_, fn ... -> ... <Repo>.<one|all|get|get_by|aggregate|exists?|stream> ... end)`
- `Enum.map(_, &<Repo>.<...>(&1, ...))` (point-free form is the same trap)
- `Enum.each(_, fn ... -> ... <Repo>.<insert|update|delete> ... end)`
- `Enum.flat_map`, `Enum.reduce(..., fn _, acc -> ... <Repo>... end)`
- `for x <- xs, do: <Repo>.*` / `for x <- xs, do: ...` with a query inside
- `Enum.map(_, &<Repo>.preload(&1, ...))` — `preload/2` already accepts a list
- Pipelines like `xs |> Enum.map(&fetch_thing/1)` where `fetch_thing/1`
  internally calls a `Repo` — follow the function one hop in.

The repos to watch: `Tuist.Repo`, `Tuist.ClickHouseRepo`,
`Tuist.IngestRepo`, plus any aliased form (e.g. `alias Tuist.Repo`,
then bare `Repo.*` inside the loop).

### Flag (Severity: medium; high if hot path)

- A `Repo`/`ClickHouseRepo`/`IngestRepo` read or aggregate inside any
  `Enum.*`/`for`/`Stream.*` in the diff. Severity is **high** if the
  loop is on a request path (controller, LiveView mount/handle_*,
  channel, MCP handler) or scales with tenant data (test cases,
  bundles, runs); **medium** for background jobs and one-shot scripts.
- Per-element `Repo.preload/2` — `preload` already takes a list; one
  call covers all.
- Per-element inserts/updates/deletes that have an `_all` equivalent
  (`insert_all`, `update_all`, `delete_all`).

When suggesting a fix, **name the consolidating primitive** so the
author can act on it directly:

- ClickHouse per-bucket aggregation → `argMaxIf` / `countIf` /
  `groupArray` over a single GROUP BY, or `arrayJoin` to fan out
  buckets as rows.
- Ecto per-row lookup → `where: r.id in ^ids` + group in Elixir, or
  a join.
- Per-element preload → `Repo.preload(list, [:assoc])` once.
- Per-element write → `*_all` + a list of params.

### Do not flag

- Loops over a bounded constant collection (config keys, enum members,
  ≤5 items) where each query is genuinely independent and the loop
  isn't on a hot request path.
- Tests, fixtures, and seed scripts (`server/test/`,
  `server/priv/repo/seeds*.exs`) — correctness-first, perf is fine.
- Loops that build params in memory with no DB round trip per iteration.
- `Repo.stream/2` inside `Enum.*` with an explicit comment justifying
  the cursor-based stream (e.g. "stream so we don't load 10M rows").
- Pre-existing N+1s untouched by the diff — this skill is for new
  regressions, not codebase-wide audits.

---

## 10. Inline `style="..."` in HEEx templates

Component styling lives in `server/assets/app/css/pages/*.css` (or
`noora/lib/noora/**/*.css` for design-system primitives), keyed off
`data-part` selectors that mirror the HEEx structure. Inline `style=`
attributes on elements or component props bypass the design tokens
(`var(--noora-spacing-*)`, `var(--noora-font-*)`, etc.) at review
time, leak presentation into LiveView diffs, and prevent themers /
density modes from overriding the value.

### Flag (Severity: low)

- A new `style="..."` attribute on an HTML element inside any
  `server/lib/tuist_web/**/*.html.heex` or `*_live.html.heex`.
- A `style=` prop passed to a Noora component (`<.button_group
  style="...">`, `<.text_input style="...">`, etc.). These flow through
  to the underlying element via `{@rest}`, so they're inline styles by
  another name.

When suggesting a fix:

1. Add a stable `data-part` (or reuse one already on the element).
2. Move the rule into the matching page CSS file
   (`server/assets/app/css/pages/<page>.css`) or, if it belongs to a
   reusable component, the Noora primitive's CSS.
3. Prefer Noora design tokens (`--noora-spacing-*`, `--noora-radius-*`,
   `--noora-font-*`, `--noora-surface-*`) over raw values.

### Do not flag

- `style=` attributes that already existed before the diff.
- Generated SVG markup with inline styles (it's the artist tool's
  output, not author-written).
- One-off `style="display: none"` toggles whose visibility is driven
  by a temporary Phoenix `:if` — those still belong in CSS, but the
  signal-to-noise here is low.

---

## 11. Marketing changelog for user-facing server/dashboard features

The human-authored product changelog lives in
`server/priv/marketing/changelog/*.md`. Generated files such as
`server/CHANGELOG.md` or root `CHANGELOG.md` must not be edited by
authors.

### Flag (Severity: medium)

- A PR that adds or materially changes a user-facing server/dashboard
  feature without also adding or updating a
  `server/priv/marketing/changelog/*.md` entry.
- A PR that adds or updates a `server/priv/marketing/changelog/*.md`
  entry for a feature that is ops-only, admin-only, feature-flagged only
  for internal rollout, infrastructure-only, or otherwise not meant to
  be announced to customers yet.

User-facing signals include changed dashboard routes, LiveViews,
controllers, templates, page CSS, settings pages, integration flows,
alerts, reports, previews, build/test/cache/bundle analytics, or public
API behavior that customers can observe.

Do not treat a dashboard/UI change as announceable only because it lives
in user-facing code. If the diff gates the behavior behind an
account/org feature flag, ops/admin-only access, or an explicit internal
rollout path, it is not ready for the product changelog unless the PR
also makes that behavior broadly available to customers.

Do not request a product changelog for fix PRs. This includes fixes that
add or adjust dashboard fields, copy, validation, or settings controls
when those UI changes are part of making an already-announced or already
shipped flow work correctly. Only ask for a changelog when the PR's
primary purpose is to launch a new customer-facing capability, not when
the PR is repairing or completing a broken flow.

When suggesting a fix, ask for a short marketing changelog entry with
frontmatter like `title`, `category: "Product"`, and `pull_request`.
Mention an accompanying image under
`server/priv/static/marketing/images/changelog/` only when the feature
has a visual dashboard/UI state worth showing.

When flagging an inappropriate changelog entry, suggest removing the
entry and, if the work still needs coordination, tracking it in the PR
description or internal release notes instead.

### Do not flag

- Bug fixes with no new or materially changed user-facing behavior.
- Fix PRs, even when the fix includes small user-facing UI changes needed
  to make an already-shipped flow work correctly.
- Refactors, performance work, infrastructure, ops/admin-only paths,
  internal jobs, telemetry-only changes, tests, fixtures, or schema-only
  plumbing whose effect is not directly visible to customers.
- Features gated behind account/org feature flags that are being used
  for internal rollout or controlled access, unless the PR also makes
  the feature generally available to customers.
- Documentation-only or marketing-only PRs.
- CLI/app/cache/kura/noora-only changes. This rule is for
  user-facing server/dashboard features.
- PRs that already add or update a matching
  `server/priv/marketing/changelog/*.md` entry.

---

## 12. Changeset functions must have tests

Ecto changeset functions in schema modules under `server/lib/tuist/`
(`def changeset/N`, `def create_changeset/N`, `def update_changeset/N`,
or any other `*_changeset/N`) encode validation and persistence
contracts. New or materially changed changeset bodies that ship without
tests regress silently: a deleted `validate_*`, a widened `cast` list,
or a missing `unique_constraint` won't fail CI — the first signal is a
production error or a malformed row.

The convention in this repo is one `<schema>_test.exs` per schema module
that asserts on `errors_on(changeset)` for invalid inputs and
`changeset.valid?` for valid ones — see
`server/test/tuist/projects/project_test.exs` and
`server/test/tuist/cache_action_items/cache_action_item_test.exs` for
canonical examples.

### Flag (Severity: medium)

- A new `def changeset(`, `def create_changeset(`, `def update_changeset(`,
  or any `def *_changeset(` added in a `server/lib/tuist/**/*.ex` file
  whose diff does **not** also add at least one test case calling that
  function (e.g. `WebhookEndpoint.create_changeset(...)`,
  `Project.update_changeset(...)`) in `server/test/tuist/**/*_test.exs`.
- A materially changed changeset body — a new or modified `cast`,
  `validate_required`, `validate_length`, `validate_format`,
  `validate_inclusion`, `validate_change`, `unique_constraint`,
  `foreign_key_constraint`, or `put_change` line in the diff — where
  the change isn't exercised by a test added or modified in the same
  diff.

When flagging, name the schema module and point to a sibling
`<schema>_test.exs` as the place to add coverage. If no such test file
exists, request its creation alongside the changeset.

### Do not flag

- Trivial mechanical edits (renaming a field already covered by an
  existing test, removing a single `cast` field, formatting-only churn,
  reordering pipe steps inside an unchanged body).
- Changeset functions in `server/test/support/` fixtures or
  `server/priv/repo/seeds*.exs`.
- Diffs that exercise the changeset indirectly through a higher-level
  context test (e.g. `accounts_test.exs` calling
  `Accounts.create_user/1`, which in turn invokes the changeset) — that
  counts as coverage. Only flag when the diff has *no* test reference
  to the schema module or its changeset functions.
- Changeset edits that are purely a consequence of a column rename
  already covered by a migration-level test or by an existing
  `errors_on(changeset)` assertion that still passes against the new
  field name.

---

## Out of scope (handled elsewhere — do not flag)

- Module / function naming, pipe-chain start, function ordering,
  parentheses-on-no-arg-calls → `mix format` + `credo` (`PipeChainStart`,
  `StrictModuleLayout`, `Nesting`, `UnsafeToAtom`, `ModuleDoc`).
- Inline `import` / `alias` / `require` inside a `def`/`defp`/`test`/
  `setup`/`describe` body → covered by the custom credo check
  `Credo.Checks.DisallowDirectivesInFunction`.
- Missing `@spec` / `@type` — this codebase intentionally avoids
  typespecs. Never suggest adding them.
- Missing `@doc` / `@moduledoc` on internal helper modules.
- `String.to_atom/1` on user input — credo's `UnsafeToAtom` covers it.

## Before submitting findings

For each finding, confirm:

1. The `path:line` is real and the snippet appears in the diff.
2. The category above is one of 1–12; if it isn't, downgrade to a
   question (`uncertain: ...`) rather than asserting a finding.
3. The severity is set: **critical** (auth bypass / cross-tenant read or
   write), **high** (likely security or correctness bug), **medium**
   (compliance / consistency gap), **low** (nice-to-have).
4. You are not reporting an unchanged line as a finding. Unchanged
   context can explain a diff finding, but cannot be the finding itself.