Code Review And Quality

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.
• 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.
• 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 useBreakpoint / useIsMobile from apps/chat/src/hooks/breakpoint/useBreakpoint.ts, 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.