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: | Mode | Use when | Required context | | ------------------- | ----------------------------------- | ---------------------------------------------------------------------------------------------- | | **Local review** | Reviewing uncommitted local changes | `git status`, `git diff`, full changed files, related OpenSpec artifacts if any | | **PR review** | Reviewing a GitHub PR number or URL | PR metadata, PR diff, full changed files at PR head, related OpenSpec artifacts | | **Self-review** | Finishing an implementation slice | Touched files, completed task, tests run, remaining task status | | **OpenSpec review** | Reviewing an OpenSpec-backed change | `proposal.md`, `design.md`, `tasks.md`, changed specs, implementation diff | | **Pipeline review** | CI/bot review of a PR diff | Base/head refs, PR diff, full changed files, related OpenSpec artifacts, relevant check output | 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: ```json { "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: ```markdown 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. ## Repo-specific routing Use the repository skills and rules as the source of truth before applying generic advice: | Change area | Read / apply | | -------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------- | | Workspace structure, project ownership | `openspec/config.yaml`, `AGENTS.md`, `.agents/skills/nx-workspace/SKILL.md` | | Multi-file implementation or refactor | thin vertical slices + per-slice verify (`openspec/config.yaml` task rules) | | HTTP API contract or generated client | `.agents/skills/api-design/SKILL.md` | | `apps/chat-api/**` | `apps/chat-api/AGENTS.md` | | `libs/*` React components | `openspec/config.yaml`, library isolation rules from `AGENTS.md`, `openspec/lib-styling-guide.md` plus exported-symbol JSDoc rules | | UI kit components | Use the `@epam/ai-dial-ui-kit` MCP tools before recommending raw HTML primitives | | Responsive / mobile parity | `.claude/skills/responsive-design/SKILL.md` | | READMEs, `docs/**`, lib public API | `.claude/rules/docs.md` plus `npm run validate:docs`; use the `dial-docs` skill to find the authoritative doc | | CI status or self-healing fixes | `.agents/skills/monitor-ci/SKILL.md`; do not replace it with ad hoc polling | 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 | Size (approx.) | Expectation |