REVIEW.md

Rama main · réplica de lectura del repositorio original.

# Code Review Guidelines

The keywords "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this specification are to be interpreted as described in BCP 14 [RFC2119] [RFC8174] when, and only when, they appear in all capitals, as shown here.

An implementation is compliant if it satisfies all "MUST", "MUST NOT", "REQUIRED", "SHALL", and "SHALL NOT" requirements defined in this specification. An implementation that fails to satisfy any such requirement is not compliant.

## Always Check

- New API endpoints MUST have corresponding tests and authorization checks
- Database schema changes MUST include migrations and MUST be tested across all supported backends
- Error messages MUST NOT leak internal details to users — typed error classes SHOULD be used for expected failures
- Data-mutating operations MUST emit events so downstream subscribers stay in sync
- Batch loading MUST be used to avoid N+1 query patterns in resolver-style APIs
- Tests SHOULD extend existing test modules unless they cover a distinct responsibility
- Tests MUST fail when the behavior they cover is removed — tests that only exercise mocks or restate the implementation MUST NOT be added
- Docs the change makes stale MUST be updated in the same PR; new environment variables MUST be documented and new docs pages indexed in `docs/phoenix/llms.txt`
- Generated files (`__generated__` directories, GraphQL and OpenAPI schemas, the model cost manifest, the sitemap) MUST be regenerated by their `make` target or workflow, never edited by hand
- Changes to packages under `js/packages/` MUST include a changeset

## Style

- Variable names MUST be descriptive — short names like `x` or `cv` MUST NOT be used. Use names like `num_experiments` or `current_version`
- Variable names SHOULD use prefixes to denote the type — e.g. `num_records` rather than `records`
- Functions MUST be named with precise action verbs — e.g. `list_sessions()` rather than `sessions()`
- TypeScript functions with two or more parameters MUST take a single destructured object parameter — e.g. `divide({ numerator, denominator })` rather than `divide(x, y)`. Callbacks whose signature is fixed by a library or the DOM are exempt
- Python functions SHOULD take keyword arguments over positional ones for clarity — e.g. `divide(*, numerator, denominator)` rather than `divide(x, y)`
- Composition SHOULD be preferred over inheritance
- All functions MUST have type annotations — lean on the type system to catch bugs at compile time, not runtime
- Path aliases or absolute imports SHOULD be used — deep relative import chains are NOT RECOMMENDED
- Named exports SHOULD be preferred over default exports
- Types SHOULD be as precise as the data — `Literal` types and discriminated unions over bare strings, exhaustive matching (`assertUnreachable`, `assert_never`), and no widening to nullable
- Comments SHOULD explain why, not what — comments that narrate the code, address the reviewer, or describe how the change was made MUST NOT be added

## Patterns to Flag

- Catching broad exception types without re-raising MUST NOT occur — specific exceptions MUST be caught
- Direct database queries inside API resolvers MUST NOT be used — batched data access patterns are REQUIRED
- Endpoints that modify data MUST have authorization checks
- Queries or logic MUST work on all supported database backends — single-dialect code MUST NOT be introduced
- State SHOULD NOT be derived in side effects when it can be computed directly
- Cache or store mutations MUST update affected collections to prevent stale lists in the UI
- Form fields MUST have validation rules on required inputs
- Existing helpers, generated types and constants MUST be reused rather than reimplemented
- Abstractions with a single call site SHOULD NOT be introduced
- Public API parameters, config options and exports the change does not need MUST NOT be added
- Code, props and exports the change leaves unused MUST be removed
- Modules MUST live in the layer they belong to — `utils` holds pure functions only, and imports SHOULD flow one way
- Code MUST NOT branch on hard-coded model names, model ID substrings, or version strings to decide how to call a provider, and SHOULD NOT send a request it expects some models to reject and then retry it in another form. When models disagree on a parameter, the request SHOULD use the form every supported model accepts. Name lists go stale with every model release and miss aliases, deployment IDs, and proxied model names

## Security

- GraphQL resolvers that perform side effects — outbound HTTP/RPC, "test connection" / credential validation, database writes, secret reads, filesystem writes, or any operation that takes a user-supplied URL, hostname, or set of HTTP headers — MUST be exposed as `@strawberry.mutation` (or `@strawberry.subscription`), NEVER as a `Query` field. Query fields are reachable through introspection, are not covered by the `make check-graphql-permissions` CI guard, and have historically been used to bypass auth (SSRF against internal services, including cloud metadata endpoints).
- All `@strawberry.mutation` and `@strawberry.subscription` definitions MUST declare `permission_classes=[...]` — at minimum `IsNotReadOnly`, `IsNotViewer`, and (where appropriate) `IsAdminIfAuthEnabled` and/or `IsLocked`. The `make check-graphql-permissions` script enforces this in CI; do not bypass it.
- User-supplied URLs, hostnames, base URLs, or endpoint overrides that the server will dial out to MUST be treated as untrusted. They MUST NOT be allowed to resolve to link-local, loopback, private, or cloud metadata addresses (e.g. `169.254.169.254`, `metadata.google.internal`, `127.0.0.0/8`, `10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`, `::1`, `fc00::/7`, `fe80::/10`) unless the feature explicitly requires it and is gated behind admin-only auth.
- User-supplied HTTP headers and client kwargs MUST NOT be forwarded to internal or privileged services without an allowlist. Headers like `Metadata-Flavor`, `X-Forwarded-For`, `Authorization`, and `Host` are particularly sensitive.
- Secrets (API keys, tokens, encrypted config) MUST NOT appear in error messages, log lines, GraphQL error payloads, or telemetry. Error messages from upstream providers MAY be surfaced verbatim only when the upstream is the user's own configured provider; otherwise they MUST be sanitized.
- GraphQL introspection-discoverable fields that perform privileged operations MUST have a regression test that asserts both the schema location (mutation, not query) and the presence of the required `permission_classes`. See `tests/unit/server/api/mutations/test_generative_model_custom_provider_mutations.py::test_test_credentials_is_an_auth_gated_mutation_not_a_query` for the canonical pattern.
- Authentication and authorization checks MUST NOT be implemented only on the frontend. Server-side enforcement is REQUIRED for every privileged operation.

## Reporting Findings

- Each finding MUST be verified against the code before it is reported: quote the line, and for a bug name the input or state that triggers it
- Only problems the change introduces SHOULD be reported — pre-existing issues and anything a linter, formatter or type checker catches MUST NOT be
- Each root cause MUST be reported once, listing every location; generated-file drift is reported at its source
- Each finding MUST carry a severity: **Blocking** (wrong behavior, security, data loss, or a broken MUST) or **Non-blocking** (a broken SHOULD). Nits and praise SHOULD NOT be posted
- A convention finding MUST cite the rule it breaks in this file or in `.agents/skills/`
- A finding SHOULD fit in five lines: what is wrong, the evidence, and a concrete fix

## Skip

- Generated files under `__generated__` directories
- Files under `node_modules/`, `dist/`, or `build/`