From fd81d77ae36b58d4caba52177082e7f7a2d260df Mon Sep 17 00:00:00 2001 From: "Juan S. Mrad" Date: Tue, 12 May 2026 22:58:28 -0500 Subject: [PATCH 1/5] Define AI Code review configurations and instructions --- .coderabbit.yaml | 78 +++++++++++++++++++++++++++++++++ .github/copilot-instructions.md | 60 +++++++++++++++++++++++++ 2 files changed, 138 insertions(+) create mode 100644 .coderabbit.yaml create mode 100644 .github/copilot-instructions.md diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 00000000..8e44fe88 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,78 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +# +# CodeRabbit configuration for Coop. Schema: +# https://docs.coderabbit.ai/reference/configuration +# +# Review guidance lives in `.github/copilot-instructions.md` and `AGENTS.md`. +# CodeRabbit's Code Guidelines feature picks both files up automatically (their +# default `knowledge_base.code_guidelines.file_patterns` includes +# `.github/copilot-instructions.md` and `**/AGENTS.md`), so we do not duplicate +# the prose here. Update the markdown files; CodeRabbit will follow. +# +# This file only carries settings that cannot be expressed in markdown: +# review profile, auto-review behavior, path filters, and which of CodeRabbit's +# bundled tools we want enabled. + +language: en-US +early_access: false + +reviews: + profile: chill + request_changes_workflow: false + high_level_summary: true + poem: false + review_status: true + collapse_walkthrough: true + + auto_review: + enabled: true + drafts: false + base_branches: + - main + + # Skip generated artifacts, lockfiles, and vendored content. These are either + # codegen output (`npm run generate`) or not human-authored. + path_filters: + - '!**/generated.ts' + - '!**/package-lock.json' + - '!**/node_modules/**' + - '!**/dist/**' + - '!**/build/**' + - '!**/*.snap' + + # ESLint and Prettier already run in CI (see AGENTS.md > "CI"). Disable the + # bundled formatters/linters so CodeRabbit does not duplicate that output. + # Keep security-oriented tools (secret scanning, SAST, IaC) enabled. + tools: + eslint: + enabled: false + biome: + enabled: false + prettier: + enabled: false + markdownlint: + enabled: false + oxc: + enabled: false + yamllint: + enabled: false + hadolint: + enabled: false + shellcheck: + enabled: false + languagetool: + enabled: false + ast-grep: + enabled: false + gitleaks: + enabled: true + semgrep: + enabled: true + checkov: + enabled: true + +knowledge_base: + # Code Guidelines is on by default; stated explicitly so a future reader sees + # that the substantive review rules come from the markdown files above. + code_guidelines: + enabled: true diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..66f734f6 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,60 @@ +# Code review instructions + +These instructions apply to AI tools when they review pull requests in this repository, and when they answer questions about this codebase. They are guidance, not a checklist use judgment, prefer fewer high-signal comments over many low-signal ones, and skip points that don't apply to the diff in front of you. For human contributor guidance see [`README.md`](../README.md); for general agent rules see [`AGENTS.md`](../AGENTS.md). + +## Repository at a glance + +- Node (`.nvmrc`), TypeScript throughout. +- Four independent packages, **not an npm workspace**: `/` (root), `/server` (Express + Apollo GraphQL, ESM), `/client` (React + Vite + Apollo Client, Ant Design + Tailwind), `/db` (Postgres/ClickHouse/Scylla migration runner), `/migrator` (CLI). +- Server uses BottleJS dependency injection wired in `server/iocContainer/`. +- GraphQL is authored inline in resolvers with `/* GraphQL */` markers and compiled by `npm run generate` into `client/src/graphql/generated.ts` and `server/graphql/generated.ts`. Both `generated.ts` files are codegen output. + +## Scope of review — focus on quality and security + +Lint and formatting are enforced by ESLint and Prettier in CI (`docker compose run --rm backend npm run lint`, `docker compose run --rm client npm run lint`), so please skip: + +- formatting, whitespace, indentation, quote style, or import ordering +- ESLint or Prettier rule violations +- typos in comments or doc grammar nits +- missing JSDoc on internal helpers +- subjective style preferences not codified in a project rule + +If a finding would be caught by `npm run lint` or `npm run format`, it's redundant. + +## Security + +Security findings are the highest-value comments you can leave. When you spot one, name the risk concretely and suggest a fix. Areas worth paying attention to: + +- **Hard-coded secrets.** API keys, tokens, passwords, OAuth secrets, JWT signing keys, DB connection strings, or webhook secrets in source or committed config. Prefer environment variables or a secret manager, fetched close to use. +- **Injection.** String-built SQL, shell commands, file paths, HTML, or LLM prompts are usually a smell. Look for parameterized queries (Knex bindings, prepared statements), argv arrays for `child_process` (avoid `shell: true`), context-aware HTML encoding, and a clear separation between trusted system prompts and untrusted user content. Raw SQL in `server/clickhouse/` and Cassandra calls in Scylla code should still use bound parameters. +- **Unvalidated input.** GraphQL resolver arguments, REST handler bodies, query params, file uploads, and headers benefit from server-side validation beyond schema-shape checks — lengths, ranges, allowlists, canonicalization. Be wary of null bytes and un-normalized Unicode in comparisons. +- **Authorization gaps / IDOR.** When a resolver, mutation, or route touches user- or tenant-scoped data, check that the caller is allowed to access _that specific resource_, not just that they're logged in. Accepting an ID from the client without an ownership/permission check is worth calling out. +- **Auth, session, CSRF, CORS, or rate-limit changes** in `server/api.ts` or request middleware are security-sensitive (`AGENTS.md` requires a maintainer for these). Worth flagging even when the diff is small. +- **Sensitive logging.** Secrets, JWTs, full `Authorization` headers, full request/response bodies, or PII in logs, traces, metrics labels, or error responses are risky. Error responses shouldn't leak stack traces. +- **Weak crypto.** MD5 or SHA-1 used for security, ECB mode, reused IVs, `Math.random()` for tokens or IDs (prefer `crypto.randomBytes` or `crypto.randomUUID`), or hand-rolled crypto are all worth questioning. JWTs should reject the `none` algorithm, use strong secrets, and have short access-token expirations. +- **Token storage.** Auth tokens generally belong in HttpOnly, Secure, SameSite cookies rather than `localStorage` or `sessionStorage`. +- **Open redirects.** Redirecting to a user-supplied URL without an allowlist is a common pitfall. +- **XSS in the client.** `dangerouslySetInnerHTML`, `innerHTML`, `document.write`, unsanitized `href`/`src` derived from user input, and `javascript:` URLs are worth a look. Prefer `textContent`; sanitize with DOMPurify when raw HTML is unavoidable. +- **Unsafe deserialization or evaluation.** `eval`, `new Function`, `setTimeout`/`setInterval` with string arguments, and `yaml.load` without a safe schema are risky. +- **Removing security controls.** If a diff disables CSRF, CORS, rate limits, authentication, or authorization, ask whether it's intentional and justified. +- **Dependency changes.** Additions, removals, or upgrades in a `package.json` or `package-lock.json` (including transitive bumps) need human approval for license (Apache 2.0 compatibility) and CVE review per `AGENTS.md` > "Human-approval-required actions". Worth surfacing so reviewers don't miss them. + +## Code quality + +Use judgment here — these are patterns that tend to cause bugs or maintenance pain in this codebase: + +- **Dependency injection.** Server code generally consumes services through `server/iocContainer/` rather than importing singletons directly, since direct imports break test mocking. +- **Generated files.** `generated.ts` (client and server) is produced by `npm run generate`; hand-edits drift from the GraphQL schema. +- **GraphQL N+1.** Resolvers that query inside a loop are usually better as a batched DataLoader call or a single query. +- **Error handling.** Silently swallowed errors (`catch {}` with no log or rethrow), unhandled promise rejections, and missing `await` on a promise whose result matters tend to cause production surprises. +- **Async correctness.** `forEach` with an `async` callback doesn't await; `for...of` with `await` or `Promise.all` is usually what's intended. Worth a look when shared state is involved. +- **Type safety.** New `any`, `as unknown as`, non-null assertions (`!`) introduced to silence a real type error, or `@ts-ignore` are worth questioning. `@ts-expect-error` with a justifying comment is preferred when an escape hatch is genuinely needed. +- **Public API stability.** Removing or renaming a GraphQL type or field breaks Apollo cache and downstream consumers. Additive changes are usually safe; removals deserve a migration plan. +- **Database migrations.** Files under `db/src/scripts//` are typically forward-only, idempotent where possible, and use `CURRENT_USER` for Postgres role grants. Editing a migration that has already shipped is a red flag — a new forward migration is usually the right path. New migration filenames use the `date -u +"%Y.%m.%dT%H.%M.%S"` prefix. +- **Tests.** New behavior generally warrants a test; bug fixes generally warrant a regression test (`AGENTS.md` > "Code review"). +- **Duplicated logic.** If a helper already exists in the same package, prefer it over a parallel implementation. +- **Dead code.** Commented-out blocks and TODOs without an issue link are worth a nudge. + +## Tone + +Be specific and concise. When you flag something, name the concrete risk ("possible SQL injection via string concatenation", "missing authorization check — possible IDOR", "secret logged at info level") and, where helpful, show the fix in code. Skip nits, and stay quiet when nothing in this list applies. From 11b1187d73f53696e5b1ff36e6b83465dfed8691 Mon Sep 17 00:00:00 2001 From: Juan Mrad Date: Wed, 13 May 2026 10:40:58 -0500 Subject: [PATCH 2/5] Update .github/copilot-instructions.md Co-authored-by: Cassidy James --- .github/copilot-instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 66f734f6..335322b0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,6 +1,6 @@ # Code review instructions -These instructions apply to AI tools when they review pull requests in this repository, and when they answer questions about this codebase. They are guidance, not a checklist use judgment, prefer fewer high-signal comments over many low-signal ones, and skip points that don't apply to the diff in front of you. For human contributor guidance see [`README.md`](../README.md); for general agent rules see [`AGENTS.md`](../AGENTS.md). +These instructions apply to AI tools when they review pull requests in this repository, and when they answer questions about this codebase. They are guidance, not a checklist. Use judgment, prefer fewer high-signal comments over many low-signal ones, and skip points that don't apply to the diff in front of you. For human contributor guidance see [`README.md`](../README.md); for general agent rules see [`AGENTS.md`](../AGENTS.md). ## Repository at a glance From a70e09adc7a083ee51c0c163770115e3836b9bf2 Mon Sep 17 00:00:00 2001 From: "Juan S. Mrad" Date: Wed, 13 May 2026 11:49:24 -0500 Subject: [PATCH 3/5] cleanup tools based on spec. Ensure we ignore dependabot prs by labeling them as such --- .coderabbit.yaml | 43 +++++---------------------------- .github/copilot-instructions.md | 2 +- .github/dependabot.yml | 39 +++++++++++++++++++++++------- 3 files changed, 37 insertions(+), 47 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 8e44fe88..4473f23e 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,20 +1,8 @@ # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json # -# CodeRabbit configuration for Coop. Schema: -# https://docs.coderabbit.ai/reference/configuration -# -# Review guidance lives in `.github/copilot-instructions.md` and `AGENTS.md`. -# CodeRabbit's Code Guidelines feature picks both files up automatically (their -# default `knowledge_base.code_guidelines.file_patterns` includes -# `.github/copilot-instructions.md` and `**/AGENTS.md`), so we do not duplicate -# the prose here. Update the markdown files; CodeRabbit will follow. -# -# This file only carries settings that cannot be expressed in markdown: -# review profile, auto-review behavior, path filters, and which of CodeRabbit's -# bundled tools we want enabled. - -language: en-US -early_access: false +# Review rules live in `.github/copilot-instructions.md` and `AGENTS.md` — +# CodeRabbit picks both up automatically. This file only carries settings that +# differ from CodeRabbit's defaults. reviews: profile: chill @@ -29,9 +17,8 @@ reviews: drafts: false base_branches: - main - - # Skip generated artifacts, lockfiles, and vendored content. These are either - # codegen output (`npm run generate`) or not human-authored. + labels: + - '!dependabot' path_filters: - '!**/generated.ts' - '!**/package-lock.json' @@ -40,16 +27,12 @@ reviews: - '!**/build/**' - '!**/*.snap' - # ESLint and Prettier already run in CI (see AGENTS.md > "CI"). Disable the - # bundled formatters/linters so CodeRabbit does not duplicate that output. - # Keep security-oriented tools (secret scanning, SAST, IaC) enabled. + # Lint runs in CI; disable CodeRabbit's bundled linters to avoid duplicates. tools: eslint: enabled: false biome: enabled: false - prettier: - enabled: false markdownlint: enabled: false oxc: @@ -62,17 +45,3 @@ reviews: enabled: false languagetool: enabled: false - ast-grep: - enabled: false - gitleaks: - enabled: true - semgrep: - enabled: true - checkov: - enabled: true - -knowledge_base: - # Code Guidelines is on by default; stated explicitly so a future reader sees - # that the substantive review rules come from the markdown files above. - code_guidelines: - enabled: true diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 335322b0..9c2cea1e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -5,7 +5,7 @@ These instructions apply to AI tools when they review pull requests in this repo ## Repository at a glance - Node (`.nvmrc`), TypeScript throughout. -- Four independent packages, **not an npm workspace**: `/` (root), `/server` (Express + Apollo GraphQL, ESM), `/client` (React + Vite + Apollo Client, Ant Design + Tailwind), `/db` (Postgres/ClickHouse/Scylla migration runner), `/migrator` (CLI). +- Independent packages, **not an npm workspace** — each has its own `package.json` and lockfile. Main ones: `/` (root), `/server` (Express + Apollo GraphQL, ESM), `/client` (React + Vite + Apollo Client, Ant Design + Tailwind), `/db` (Postgres/ClickHouse/Scylla migration runner), `/migrator` (CLI). - Server uses BottleJS dependency injection wired in `server/iocContainer/`. - GraphQL is authored inline in resolvers with `/* GraphQL */` markers and compiled by `npm run generate` into `client/src/graphql/generated.ts` and `server/graphql/generated.ts`. Both `generated.ts` files are codegen output. diff --git a/.github/dependabot.yml b/.github/dependabot.yml index a0cf453e..d0c156c5 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -10,24 +10,30 @@ version: 2 updates: # Enable version updates for Docker - - package-ecosystem: "docker" + - package-ecosystem: 'docker' directories: - - "/" - - "/client" - - "/db" - - "/hma" - - "/nodejs-instrumentation" + - '/' + - '/client' + - '/db' + - '/hma' + - '/nodejs-instrumentation' schedule: - interval: "weekly" + interval: 'weekly' + labels: + - 'dependencies' + - 'dependabot' ignore: - - dependency-name: "*" - update-types: ["version-update:semver-major"] + - dependency-name: '*' + update-types: ['version-update:semver-major'] # Enable version updates for npm - package-ecosystem: 'npm' directory: '/' schedule: interval: 'weekly' + labels: + - 'dependencies' + - 'dependabot' ignore: - dependency-name: '*' update-types: ['version-update:semver-major'] @@ -52,6 +58,9 @@ updates: directory: '/db' schedule: interval: 'weekly' + labels: + - 'dependencies' + - 'dependabot' ignore: - dependency-name: '*' update-types: ['version-update:semver-major'] @@ -76,6 +85,9 @@ updates: directory: '/migrator' schedule: interval: 'weekly' + labels: + - 'dependencies' + - 'dependabot' ignore: - dependency-name: '*' update-types: ['version-update:semver-major'] @@ -100,6 +112,9 @@ updates: directory: '/server' schedule: interval: 'weekly' + labels: + - 'dependencies' + - 'dependabot' ignore: - dependency-name: '*' update-types: ['version-update:semver-major'] @@ -124,6 +139,9 @@ updates: directory: '/client' schedule: interval: 'weekly' + labels: + - 'dependencies' + - 'dependabot' ignore: - dependency-name: '*' update-types: ['version-update:semver-major'] @@ -148,6 +166,9 @@ updates: directory: '/nodejs-instrumentation' schedule: interval: 'weekly' + labels: + - 'dependencies' + - 'dependabot' ignore: - dependency-name: '*' update-types: ['version-update:semver-major'] From 790be98c8d74f779e4178515cdbea93fcb7bbf6d Mon Sep 17 00:00:00 2001 From: "Juan S. Mrad" Date: Wed, 13 May 2026 17:23:59 -0500 Subject: [PATCH 4/5] Address PR #445 review: path-scoped review, race-proof Dependabot filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Switch Dependabot filter to ignore_usernames (race-proof — the pull_request.opened webhook fires before pull_request.labeled). Keep labels: ['!dependabot'] as a manual opt-out path. - Re-enable hadolint and shellcheck — they aren't covered by CI workflows. - Move path-specific guidance from copilot-instructions.md into .coderabbit.yaml path_instructions (resolvers, server/api.ts, ClickHouse adapters, Scylla, iocContainer, migrations, client, dependency manifests). Shorter per-diff prompts for CodeRabbit; cross-cutting concerns stay in the markdown for Copilot and other readers. - Add "What not to flag" section to copilot-instructions.md with examples of low-signal comments to suppress. Co-Authored-By: Cursor Agent --- .coderabbit.yaml | 74 +++++++++++++++++++++++++++++---- .github/copilot-instructions.md | 36 +++++++++------- 2 files changed, 87 insertions(+), 23 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 4473f23e..dd10c0f1 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -17,6 +17,9 @@ reviews: drafts: false base_branches: - main + ignore_usernames: + - 'dependabot[bot]' + - 'dependabot-preview[bot]' labels: - '!dependabot' path_filters: @@ -27,7 +30,70 @@ reviews: - '!**/build/**' - '!**/*.snap' - # Lint runs in CI; disable CodeRabbit's bundled linters to avoid duplicates. + # Cross-cutting rules live in .github/copilot-instructions.md. + path_instructions: + - path: 'server/graphql/modules/**' + instructions: | + - N+1: flag `await` inside loops over user-supplied IDs. DataLoader or + a single batched query is the fix. + - IDOR / authorization: verify each resolver checks that the caller + owns or can access the specific resource ID, not just that they are + authenticated. + - Input validation: GraphQL schema shape is not enough — look for + length, range, allowlist, and canonicalization checks on resolver + arguments. + - Schema stability: removing or renaming a GraphQL type or field + breaks Apollo cache and downstream consumers. Additive changes are + usually safe; removals deserve a migration plan. + + - path: 'server/api.ts' + instructions: | + Auth, session, CSRF, CORS, and rate-limit middleware live here. + AGENTS.md requires a maintainer for changes to this file — surface + them even when the diff is small. Flag any change that disables a + security control. + + - path: 'server/**/Clickhouse*.ts' + instructions: | + Raw SQL is expected in the ClickHouse adapter files. Flag any string + interpolation of user input — bound parameters are required. + + - path: 'server/scylla/**' + instructions: | + Cassandra driver queries must use bound parameters. Flag string-built + CQL with user input. + + - path: 'server/iocContainer/**' + instructions: | + Services are registered here for BottleJS DI. Direct imports of + service singletons elsewhere bypass test mocking and should be flagged. + + - path: 'db/src/scripts/**' + instructions: | + Migrations are forward-only. Editing a migration that has already + shipped is a red flag — add a new forward migration instead. Postgres + role grants must use `CURRENT_USER`. New filenames use the + `date -u +"%Y.%m.%dT%H.%M.%S"` prefix. + + - path: 'client/**/*.{ts,tsx}' + instructions: | + - XSS: flag `dangerouslySetInnerHTML`, `innerHTML`, `document.write`, + `javascript:` URLs, and unsanitized `href`/`src` from user input. + Prefer `textContent`; sanitize with DOMPurify when raw HTML is + unavoidable. + - Token storage: auth tokens belong in HttpOnly, Secure, SameSite + cookies — flag `localStorage` or `sessionStorage` use for tokens. + - Open redirects: redirecting to a user-supplied URL without an + allowlist is risky. + + - path: '**/{package.json,package-lock.json}' + instructions: | + Dependency additions, removals, or upgrades (including transitive + bumps) require human approval for license (Apache 2.0) and CVE review + per AGENTS.md > "Human-approval-required actions". Surface every + change so reviewers don't miss it. + + # ESLint runs in CI; the others overlap with it or surface style noise. tools: eslint: enabled: false @@ -37,11 +103,5 @@ reviews: enabled: false oxc: enabled: false - yamllint: - enabled: false - hadolint: - enabled: false - shellcheck: - enabled: false languagetool: enabled: false diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 9c2cea1e..330915dd 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -21,40 +21,44 @@ Lint and formatting are enforced by ESLint and Prettier in CI (`docker compose r If a finding would be caught by `npm run lint` or `npm run format`, it's redundant. -## Security +## Security (cross-cutting) -Security findings are the highest-value comments you can leave. When you spot one, name the risk concretely and suggest a fix. Areas worth paying attention to: +Security findings are the highest-value comments you can leave. When you spot one, name the risk concretely and suggest a fix. Areas to watch across the whole codebase: - **Hard-coded secrets.** API keys, tokens, passwords, OAuth secrets, JWT signing keys, DB connection strings, or webhook secrets in source or committed config. Prefer environment variables or a secret manager, fetched close to use. -- **Injection.** String-built SQL, shell commands, file paths, HTML, or LLM prompts are usually a smell. Look for parameterized queries (Knex bindings, prepared statements), argv arrays for `child_process` (avoid `shell: true`), context-aware HTML encoding, and a clear separation between trusted system prompts and untrusted user content. Raw SQL in `server/clickhouse/` and Cassandra calls in Scylla code should still use bound parameters. -- **Unvalidated input.** GraphQL resolver arguments, REST handler bodies, query params, file uploads, and headers benefit from server-side validation beyond schema-shape checks — lengths, ranges, allowlists, canonicalization. Be wary of null bytes and un-normalized Unicode in comparisons. -- **Authorization gaps / IDOR.** When a resolver, mutation, or route touches user- or tenant-scoped data, check that the caller is allowed to access _that specific resource_, not just that they're logged in. Accepting an ID from the client without an ownership/permission check is worth calling out. -- **Auth, session, CSRF, CORS, or rate-limit changes** in `server/api.ts` or request middleware are security-sensitive (`AGENTS.md` requires a maintainer for these). Worth flagging even when the diff is small. +- **Injection.** String-built SQL, shell commands, file paths, HTML, or LLM prompts are usually a smell. Look for parameterized queries (Knex bindings, prepared statements), argv arrays for `child_process` (avoid `shell: true`), context-aware HTML encoding, and a clear separation between trusted system prompts and untrusted user content. - **Sensitive logging.** Secrets, JWTs, full `Authorization` headers, full request/response bodies, or PII in logs, traces, metrics labels, or error responses are risky. Error responses shouldn't leak stack traces. - **Weak crypto.** MD5 or SHA-1 used for security, ECB mode, reused IVs, `Math.random()` for tokens or IDs (prefer `crypto.randomBytes` or `crypto.randomUUID`), or hand-rolled crypto are all worth questioning. JWTs should reject the `none` algorithm, use strong secrets, and have short access-token expirations. -- **Token storage.** Auth tokens generally belong in HttpOnly, Secure, SameSite cookies rather than `localStorage` or `sessionStorage`. -- **Open redirects.** Redirecting to a user-supplied URL without an allowlist is a common pitfall. -- **XSS in the client.** `dangerouslySetInnerHTML`, `innerHTML`, `document.write`, unsanitized `href`/`src` derived from user input, and `javascript:` URLs are worth a look. Prefer `textContent`; sanitize with DOMPurify when raw HTML is unavoidable. - **Unsafe deserialization or evaluation.** `eval`, `new Function`, `setTimeout`/`setInterval` with string arguments, and `yaml.load` without a safe schema are risky. - **Removing security controls.** If a diff disables CSRF, CORS, rate limits, authentication, or authorization, ask whether it's intentional and justified. -- **Dependency changes.** Additions, removals, or upgrades in a `package.json` or `package-lock.json` (including transitive bumps) need human approval for license (Apache 2.0 compatibility) and CVE review per `AGENTS.md` > "Human-approval-required actions". Worth surfacing so reviewers don't miss them. -## Code quality +Path-specific concerns (resolvers, `server/api.ts`, client, raw SQL in ClickHouse/Scylla, migrations, dependency manifests) are scoped in [`.coderabbit.yaml`](../.coderabbit.yaml) under `reviews.path_instructions`. When reviewing those areas, apply the same general principles — ownership checks on resolver-supplied IDs, parameterized queries, XSS care on the client, license/CVE attention on dependency bumps. -Use judgment here — these are patterns that tend to cause bugs or maintenance pain in this codebase: +## Code quality (cross-cutting) + +Use judgment — these patterns tend to cause bugs or maintenance pain regardless of where they appear: -- **Dependency injection.** Server code generally consumes services through `server/iocContainer/` rather than importing singletons directly, since direct imports break test mocking. - **Generated files.** `generated.ts` (client and server) is produced by `npm run generate`; hand-edits drift from the GraphQL schema. -- **GraphQL N+1.** Resolvers that query inside a loop are usually better as a batched DataLoader call or a single query. - **Error handling.** Silently swallowed errors (`catch {}` with no log or rethrow), unhandled promise rejections, and missing `await` on a promise whose result matters tend to cause production surprises. - **Async correctness.** `forEach` with an `async` callback doesn't await; `for...of` with `await` or `Promise.all` is usually what's intended. Worth a look when shared state is involved. - **Type safety.** New `any`, `as unknown as`, non-null assertions (`!`) introduced to silence a real type error, or `@ts-ignore` are worth questioning. `@ts-expect-error` with a justifying comment is preferred when an escape hatch is genuinely needed. -- **Public API stability.** Removing or renaming a GraphQL type or field breaks Apollo cache and downstream consumers. Additive changes are usually safe; removals deserve a migration plan. -- **Database migrations.** Files under `db/src/scripts//` are typically forward-only, idempotent where possible, and use `CURRENT_USER` for Postgres role grants. Editing a migration that has already shipped is a red flag — a new forward migration is usually the right path. New migration filenames use the `date -u +"%Y.%m.%dT%H.%M.%S"` prefix. - **Tests.** New behavior generally warrants a test; bug fixes generally warrant a regression test (`AGENTS.md` > "Code review"). - **Duplicated logic.** If a helper already exists in the same package, prefer it over a parallel implementation. - **Dead code.** Commented-out blocks and TODOs without an issue link are worth a nudge. +## What not to flag + +These categories of comments tend to add noise without surfacing real risk — please skip them: + +- "Consider adding a null check" on a value already typed as non-null by TypeScript. +- "Consider adding error handling" on a wrapper that already propagates errors via `async`/`await`. +- "This could be a constant" on a string literal used in a single place. +- "Add a JSDoc comment" on an internal helper. +- Rhetorical questions like "have you considered…" without a concrete risk attached. +- Defensive-coding suggestions on values whose types already prevent the failure mode. +- "Add a test" on a config-only, doc-only, or comment-only change. +- Suggestions to rename a symbol "for clarity" without a concrete ambiguity. + ## Tone Be specific and concise. When you flag something, name the concrete risk ("possible SQL injection via string concatenation", "missing authorization check — possible IDOR", "secret logged at info level") and, where helpful, show the fix in code. Skip nits, and stay quiet when nothing in this list applies. From 1a9aae9d3036fe132ad582190b3fe079950f73dc Mon Sep 17 00:00:00 2001 From: "Juan S. Mrad" Date: Wed, 13 May 2026 22:42:26 -0500 Subject: [PATCH 5/5] address CR code review --- .coderabbit.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index dd10c0f1..6d6ee696 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -24,7 +24,6 @@ reviews: - '!dependabot' path_filters: - '!**/generated.ts' - - '!**/package-lock.json' - '!**/node_modules/**' - '!**/dist/**' - '!**/build/**'