Code review and quality

Overview

Review every non-trivial change before it lands on the main line. Use five axes: correctness, readability, architecture, security, performance — plus the two repo-specific gates below them, responsive parity and documentation accuracy.

Approval bar: Approve when the change clearly improves or preserves overall code health and matches project conventions. Do not block because you would have written it differently. Do block on real defects, security issues, or violations of agreed patterns.

When to use

• Before merge / when asked to review a change
• After implementation (self-review or cross-review)
• After bugfixes (review fix and regression coverage)
• When evaluating code produced by another agent or author

Review modes

Choose the review mode from the user's request and available context:

For PR review, read full changed files, not only diff hunks. Diffs show what changed; full files show whether the change fits the surrounding design.

Pipeline review mode

Use this mode when the review is executed by CI, a scheduled bot, or any non-interactive automation. Keep the normal five-axis review, but make the result deterministic and machine-readable.

Scope

• Review only the PR/diff scope. Do not fail the pipeline for pre-existing issues unless the PR worsens them or makes them newly reachable.
• Read full changed files and related artifacts for context, but comments/findings must point to changed lines or changed artifacts whenever possible.
• Do not run Nx build targets from this review pipeline. Treat build as a separate CI concern and only record its status when an existing CI check already provides it.
• Separate review from publishing:
  • Review step produces a structured result artifact.
  • Comment-publishing step may post inline comments and one top-level summary, but only from the structured result.

Output artifact

Emit a JSON object with this shape:

{
  "verdict": "pass | warn | fail",
  "summary": "Short human-readable summary.",
  "findings": [
    {
      "severity": "critical | required | warning | nit | optional | fyi",
      "category": "correctness | readability | architecture | security | performance | responsive | documentation | openspec | verification",
      "file": "path/from/repo/root",
      "line": 123,
      "side": "RIGHT | LEFT",
      "startLine": null,
      "startSide": null,
      "anchorable": true,
      "message": "Review comment body.",
      "blocking": true
    }
  ],
  "verification": [
    {
      "command": "npm exec nx test @epam/chat",
      "status": "passed | failed | skipped",
      "reason": "Only for failed/skipped or notable context."
    }
  ],
  "topLevelComment": "Markdown summary suitable for a PR conversation comment."
}

Use verdict: "fail" when any finding is blocking or a required verification command failed. Use verdict: "warn" only for non-blocking risks or skipped verification. Use verdict: "pass" when there are no blocking findings and required verification is green or explicitly covered by trusted CI.

file, line, side, startLine, and startSide are intended to be compatible with GitHub pull request review comments. Use side: "RIGHT" for new/head lines and side: "LEFT" only when the finding must anchor to a removed/base line. Use startLine/startSide only for multi-line comments; otherwise set them to null.

Set anchorable: true only when the finding points to a line present in the PR diff. If the issue is real but cannot be anchored to a changed line, set anchorable: false, keep file/line as best-effort context if known, and include the finding in topLevelComment instead of attempting an inline comment.

Every finding must include a non-empty message containing the full review comment body. Do not put the explanation only in custom fields, summary text, or the top-level comment; pipeline publishers use message for both inline review comments and the sticky summary table.

Pipeline fail rules

Fail the pipeline for:

• Any critical or required finding.
• Relevant non-build Nx target failure run by this review pipeline, such as test, lint, OpenAPI, or generated-client checks.
• OpenSpec drift: implementation materially diverges from proposal/design/tasks/specs.
• API/OpenAPI/generated-client contract mismatch.
• Hand-authored libs/* leaking host/external integration details.
• A color added or changed in libs/* that does not come from a *Colors prop as var(--<prefix>-x, fallback) in the component's .module.scss (a confirmed lib-theming finding on an added line), or a dial-*-text class hardcoded in lib JSX instead of a *ClassName prop default.
• User-visible text added in the diff that is not translated: a literal in lib JSX instead of a *Labels prop, a literal or raw t('…') key in apps/chat, or a new lib label field the app does not pass (a confirmed i18n-strings finding).
• Security, authz, secret exposure, data loss, or broken public contract risks.
• npm run validate:docs failure, or a lib's public API changing without its README, when the diff touches libs/*/src/index.ts, any README, or docs/**.

Do not fail the pipeline for nit, optional, or fyi findings. Use warning for non-blocking risk, missing non-critical evidence, or human-follow-up items.

Simplification and extraction findings usually use nit or optional. Use warning only when duplication or missing extraction creates meaningful maintenance risk, repeated bug-prone logic, expensive test setup, or an ownership-boundary concern. Use required only when the structure causes a concrete defect, violates library isolation, or breaks a documented architecture rule.

Comment publishing

If a pipeline job is configured to publish comments:

• Post inline comments only for critical, required, and high-signal warning findings with anchorable: true.
• Do not post inline comments for nit by default unless explicitly configured.
• Do not attempt inline comments for findings with anchorable: false; summarize them in the top-level PR comment.
• Always post one top-level PR conversation comment.
• If everything is good, post a concise positive summary instead of staying silent, for example:

Automated review passed.

- No blocking findings.
- Verification: `npm exec nx affected --target=test --base=origin/development` passed.
- Scope checked: correctness, architecture boundaries, security, performance, responsive parity, documentation accuracy, and OpenSpec alignment.

If there are findings, the top-level comment must summarize the verdict, count blocking/non-blocking findings, and list verification status. Inline comments carry the detailed code-specific feedback.

GitHub publishing step requirements:

• Add the PR head SHA as commit_id at publish time; do not require the review agent to hardcode it into findings.
• For individual inline comments, use GitHub's pull request review comment API with path, line, side, optional start_line/start_side, body, and commit_id.
• If the API rejects an inline comment because the line is not in the diff, retry once as a top-level PR comment entry and mark the finding as not anchored in the publishing log.
• For the top-level summary, use a PR conversation comment or a review body, depending on the CI integration.

OpenSpec review gate

If the change is tied to OpenSpec, review the artifacts before judging the code:

1. Identify the change from the branch, PR description, user request, or openspec list --json.
2. Read the relevant artifacts under openspec/changes/<change>/:
   • proposal.md for problem, scope, non-goals
   • design.md for architecture and local patterns
   • tasks.md for promised implementation and verification
   • changed specs under specs/**/spec.md when present
3. Check the diff against the artifacts:
   • Implementation matches the accepted scope and does not add silent scope creep.
   • Completed task checkboxes correspond to real code and tests.
   • New requirements discovered during implementation are captured in specs/design/tasks, not only in code.
   • Non-goals are still respected.
4. If implementation reveals a design/spec gap, request an artifact update before or alongside code changes.

Block merge for OpenSpec-backed work when code behavior materially diverges from the artifacts, when tasks are marked complete without implementation, or when API/user-facing requirements were implemented without updating the relevant spec/design/task.

Five-axis review

1. Correctness

• Matches spec, task, or PR description
• Edge cases: null, empty, boundaries, errors — not only happy path
• Tests exist, assert behavior, and would catch regressions
• Watch for off-by-one, races, inconsistent state

2. Readability and simplicity

• Names are specific; avoid meaningless data, result, temp
• Control flow is easy to follow; avoid unnecessary cleverness
• Abstractions earn their complexity; prefer duplication over wrong abstraction until patterns repeat
• Flag simplification opportunities: repeated logic, deeply nested code, broad functions, or verbose conditionals that would become clearer as a focused helper, hook, component, utility, or method.
• Recommend extracting reusable utilities, hooks, components, or methods when the same behavior appears in multiple places, when a local helper would clarify a complex block, or when nearby features are likely to reuse the behavior.
• Do not request extraction just because code could be abstracted; require a concrete readability, testability, reuse, or ownership-boundary benefit.
• Comments only where intent is non-obvious; remove dead code and noise

3. Architecture

• Fits existing Nx boundaries (apps/*, libs/*) and import direction
• Libraries stay host-agnostic: no host-owned integration details inside libs/*, including hardcoded /api paths, generated clients, server-api imports, app contexts, auth/session/cookie/env access, feature flags, routing/navigation, analytics/telemetry/logging clients, deployment/tenant/provider details, third-party SDK setup, platform bridges, app-specific URL schemes, or app storage keys/schemas
• Exception: libs/chat-api-client is generated by OpenAPI scripts, so generated endpoint paths, DTOs, runtime transport code, and OpenAPI artifacts are allowed there. Block hand-authored app behavior in that package and direct generated-client usage from other hand-authored libs.
• Every color in libs/* comes from a *Colors prop through a CSS custom property — var(--<prefix>-x, var(--theme-token, #hex)) in the component's .module.scss, set with buildCssVars — never a Tailwind color utility in lib JSX (text-secondary, bg-layer-raised, border-tertiary, hover:bg-*), a literal hex in lib CSS, or a bare theme token no prop can override. Typography follows the same rule: every dial-*-text class comes from a *ClassName prop (or *Typography field) default, never written straight into JSX, a module constant, or a hardcoded *ClassName passed to a child, and no raw Tailwind font utilities or font CSS. Run node .claude/skills/lib-theming/scan.mjs --base <base> on any diff that touches libs/* and verify each hit per .claude/skills/lib-theming/SKILL.md. An added line is required; a pre-existing hit in a touched file is optional.
• Every user-visible string is translated: in libs/* it comes from a *Labels prop with an English default (no react-i18next in libs), and apps/chat passes every *Labels field of a lib component it renders, built with t() and a translation-keys.ts enum member. Run node .claude/skills/i18n-strings/scan.mjs --base <base> and verify each hit per .claude/skills/i18n-strings/SKILL.md. A literal string or an untranslated label on an added line is required.
• Host/external integrations are adapted at the app edge and passed into libs through props, callbacks, resolved values, or narrow interfaces
• No unjustified new patterns; justified new ones are called out
• No sneaky circular deps or leaky module APIs
• Reusable helpers live at the right ownership level: app-specific helpers stay in apps/*, host-agnostic helpers/components may move to libs/*, and shared types stay in libs/chat-shared
• Relative .ts/.tsx imports and re-exports omit .js, .jsx, .ts, and .tsx; Vite projects use bundler resolution rather than Node ESM source specifiers
• Named finite sets of statuses, modes, variants, or lifecycle states use string enums instead of string-literal unions when exported, reused, or compared
• Duplication: only consolidate when the rule of three (or team norm) says so

4. Security

• User and external input validated at boundaries; treat external data as untrusted
• No secrets in code, logs, or repo; authz where required
• Injection-safe queries and APIs; XSS-aware rendering in UI code
• New dependencies: necessity, maintenance, size, license, npm audit awareness

5. Performance

• N+1, unbounded loops/fetches, missing pagination on lists
• UI: avoidable re-renders, huge props, sync work on hot paths
• Only flag with measurable or clear scaling reasoning when possible

6. Responsive parity

• UI changes use the project's named breakpoint prefixes (mobile: / desktop:), not nonexistent small_tablet:/large_tablet:/large_desktop:, Tailwind defaults such as sm:/md:/lg:, or arbitrary min-[…]: queries
• Authoring style is mobile-first — base classes describe the smallest supported viewport, larger bands are added via the named prefixes
• Components that branch in JS use useIsMobile from @epam/ai-dial-chat-shared, not direct window.innerWidth reads
• Touch targets meet ~44×44 CSS px on mobile; no :hover-only affordances; no horizontal scroll at 360px
• Verification story names which breakpoints were exercised — "desktop verified only" is a request-changes signal for any user-facing change
• See .claude/skills/responsive-design/SKILL.md for the full rubric

7. Documentation accuracy

Docs drift silently — no build breaks when a README documents a component that was renamed two releases ago. Nothing in lint/test/build covers it, so the review is the only gate.

• npm run validate:docs is green. It checks README coverage and H1/package identity, lib package.json metadata (description, license), that every relative markdown link resolves, and that every name a lib README imports from its own package is actually exported. Treat a failure as required.
• A change to a lib's src/index.ts — added, renamed, or removed export — carries a matching README change in the same diff.
• A renamed prop, a changed prop type, or a newly required prop is reflected in every README example that passes it.
• README code fences name only symbols that exist, with the required props present, the right value types, and imports from the package that actually exports the name (CatalogEntityType is @epam/ai-dial-chat-shared, not @epam/ai-dial-catalog).
• Prose describes what the code does today, not an intended capability. A claimed feature the component lacks is required, not a nit — readers act on it.
• Structural additions (lib, app, backend domain, context, route, ApiEndpoints entry) update docs/architecture.md; new or removed environment variables update apps/chat-api/README.md and .env.template.
• Deleting a doc means fixing every link to it, including the dial-docs skill index.
• See .claude/rules/docs.md for the same-change update matrix and the drift classes already found here.

Repo-specific routing

Use the repository skills and rules as the source of truth before applying generic advice:

Do not import generic standards that conflict with these repo rules. For example, do not require a new REST response envelope, direct frontend REST helpers, raw HTML controls, or a generic project structure when local conventions say otherwise.

Code quality standards

Use these as cross-cutting checks after applying repo-specific rules:

• Prefer clear, specific names over generic data, result, temp, item when the domain is known.
• Keep control flow shallow with early returns or extracted helpers when nesting hides the main path.
• When new code repeats behavior across files or components, call out whether it should become a reusable utility, hook, component, or method. Keep the suggested location consistent with ownership boundaries.
• When a single function grows to mix several responsibilities, suggest extracting the smallest named helper that makes the main path easier to read and test.
• Avoid functions that mix validation, IO, transformation, and presentation unless the surrounding pattern already does so.
• Avoid magic numbers; name domain thresholds, debounce delays, limits, and TTLs.
• Avoid mutation of shared state. Local mutation is acceptable only when contained, intentional, and clearer or measurably faster.
• Comments should explain why a choice exists, not restate what the code does.
• React components should expose event callback props as onEvent and name internal handlers handleEvent.
• No console.log in application code; use the app's logging pattern.
• No TODO/FIXME in merge-ready code unless linked to an accepted follow-up and non-blocking by design.
• Tests should assert observable behavior and meaningful edge/error paths, not implementation details.

Change sizing

Never mix large refactor with new behavior in one changeset unless team explicitly allows.

Comment severity

Use a prefix so authors know what is mandatory:

In pipeline JSON, use lowercase severity values: critical, required, warning, nit, optional, fyi.

Review process

1. Context — What does this change do? Which spec/task? Expected behavior?
2. OpenSpec gate — If applicable, read artifacts and compare implementation to proposal/design/tasks/specs
3. Tests first — Coverage, names, edge cases, behavioral (not brittle implementation) assertions
4. Implementation — Walk files with the five axes
5. Findings — Every item labeled with severity above
6. Verification story — What was run, what still needs a human check (UI screenshots if relevant)
7. Verdict — Approve, comment, request changes, or block

This workspace (Nx)

When checking "tests / build / lint":

• Prefer npm exec nx test <project>, npm exec nx lint <project>, npm exec nx build <project> for touched projects (see openspec/config.yaml and AGENTS.md).
• For broad checks, use affected targets with the repository base branch: npm exec nx affected --target=<target> --base=origin/development.
• In CI pipeline review mode, do not run npm exec nx build ... or npm exec nx affected --target=build ...; rely on dedicated CI build jobs if build evidence is needed.
• Do not use origin/main as the affected base in this workspace.
• When unsure which project owns a path, use npm exec nx show projects or npm exec nx show project <name> --json.

Validation matrix

CI / pipeline review mode: never run build targets. In CI, skip every build command in the table below regardless of change type. Rely on dedicated CI build jobs; record their status from existing CI check output instead.

Select the smallest validation set that proves the change:

Record skipped checks with a reason. A review without a verification story is incomplete.

Decision policy

Use a clear verdict:

For pipeline JSON, map these to pass, warn, or fail:

• pass: equivalent to Approve.
• warn: equivalent to Approve/comment or Comment only with non-blocking findings.
• fail: equivalent to Request changes or Block.

Commit / PR description

• Subject: imperative, informative alone in history ("Add retry to chat API client" not "Fix stuff").
• Body: what, why, tradeoffs, links to tasks/issues when useful.
• Weak subjects: "Fix bug", "WIP", "Phase 1", "Small changes".

Review checklist

Use as a literal template when writing a review:

## Review: [title]

### Context

- [ ] I understand intent and expected behavior
- [ ] I read related OpenSpec artifacts, or confirmed none apply

### Correctness

- [ ] Matches spec/task
- [ ] Edge and error paths
- [ ] Tests adequate and meaningful

### Readability

- [ ] Clear names and flow
- [ ] No unnecessary complexity

### Architecture

- [ ] Fits monorepo boundaries and patterns
- [ ] Hand-authored `libs/*` remain isolated from host/external integration details
- [ ] Every lib color added in the diff comes from a `*Colors` prop as `var(--<prefix>-x, fallback)` and every lib typography class from a `*ClassName` prop default (`lib-theming` scanner run, hits verified)
- [ ] Every user-visible string added in the diff is translated (`i18n-strings` scanner run, hits verified)
- [ ] `libs/chat-api-client` changes, if any, are generated OpenAPI client changes
- [ ] Coupling and abstraction level appropriate
- [ ] API/generated-client/OpenSpec contract rules followed when relevant
- [ ] Relative TypeScript source imports are extensionless
- [ ] Named finite TypeScript value sets use string enums where the project convention applies

### Security

- [ ] No secrets; boundaries validated; auth as needed
- [ ] New deps justified

### Performance

- [ ] No obvious N+1 / unbounded work / UI hot-path issues

### Responsive parity

- [ ] Uses project's named breakpoint prefixes; mobile-first authoring
- [ ] JS branches go through `useIsMobile`, not `window.innerWidth`
- [ ] Touch targets, hover-only affordances, and 360px overflow checked
- [ ] Verification names the breakpoints exercised

### Documentation accuracy

- [ ] `npm run validate:docs` green
- [ ] Public API changes (`libs/*/src/index.ts`, prop renames, new required props) reflected in the lib README in this diff
- [ ] README examples name only existing symbols, include required props, and import from the owning package
- [ ] Prose describes current behavior, not intended behavior
- [ ] `docs/architecture.md` updated for structural changes; `apps/chat-api/README.md` + `.env.template` for env vars
- [ ] No links left pointing at a deleted or renamed doc

### Verification

- [ ] Relevant Nx targets (or CI) green
- [ ] OpenAPI/generated-client checks run when API contracts changed
- [ ] `npm run validate:docs` run when READMEs, `docs/**`, or a lib's public API changed
- [ ] Manual / visual check noted if UI

### Verdict

- [ ] Approve | [ ] Request changes (list blocking items)

Rationalizations to reject

Red flags

• Merge with no real review or evidence
• Only glancing at tests, ignoring other axes
• Huge PR with "no time to split"
• Bugfix without regression test
• Comments without severity — author cannot prioritize
• Rubber-stamp "LGTM"

After review

• All Critical / Required items closed or explicitly deferred with reason
• Tests and build (for touched scope) are green
• OpenSpec tasks/specs/design reflect the implementation state
• Verdict and severity-labeled notes are clear for the author