From 2aab2e57cdfee7eb1d0f19cdb0ca413ac973129e Mon Sep 17 00:00:00 2001 From: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com> Date: Tue, 28 Apr 2026 10:17:01 -0400 Subject: [PATCH 1/2] chore: add Copilot PR review instructions Adds .github/copilot-instructions.md to guide GitHub Copilot's PR review behavior toward high-signal feedback and away from CI-duplicate noise. Process: - Reviewed Copilot's review-platform constraints (4000-char base-branch read, Comment-only review, no merge gating, no external link following) plus Google/Microsoft/OWASP/NIST review literature. - Analyzed 319 Copilot inline comments across the last 150 dealbot PRs to identify which areas Copilot reviews well (job-state consistency, test/fixture-contract drift, multi-network behavior, quoted SQL identifiers, redaction) versus where it overreaches (generic-SQL assumptions on ClickHouse code in PRs #438 and #485, low-priority frontend optimization comments). - Iterated through rounds of adversarial review (self-review against the evidence, then a second-opinion review by Codex) to tighten wording, fit the 4000-byte budget, and encode dealbot-specific invariants. Encoded: - Repository context: monorepo layout, Postgres = source of truth, ClickHouse cluster/schema/migrations owned by an external team (dealbot reviews payload correctness and operational impact, not schema/retention/ops design). - Core invariants: at most one job per SP per check type per network; jobs fail only on execution failure, not on negative check results; scheduling/cleanup/filtering/queue execution stay consistent across the same SP set and network. - Blocker/Important priorities aligned to observed high-value comment themes. - Do-Not-Comment list to suppress CI-duplicate noise (Biome, build, typecheck, test, Docker already enforced in CI). Final size: 3890/4000 bytes. --- .github/copilot-instructions.md | 64 +++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 .github/copilot-instructions.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..8f868773 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,64 @@ +# Pull Request Review Instructions + +## Goal + +Review for material correctness, security, state-consistency, compatibility, or maintainability regressions. Optimize for high-signal feedback. Prefer silence over speculation. + +## Review Philosophy + +- Comment only when confident a real issue exists in the changed code or its immediate context. +- Prioritize issues as `Blocker` or `Important`. +- Prefer incremental improvement over idealized rewrites. Do not request broad refactors unless needed to prevent a defect. +- Focus on issues the author can reasonably address in this PR. +- If the PR is too large to review confidently, leave at most one comment asking for a smaller PR. +- If uncertain, do not comment. + +## Dealbot Context + +- TypeScript monorepo: NestJS backend (`apps/backend`), React/Vite frontend (`apps/web`), subgraph (`apps/subgraph`). +- Postgres is the source of truth for operational state. +- ClickHouse stores telemetry events. The cluster, schema, and migrations are owned by an external team. Review ClickHouse-touching code for dealbot impact (event payload shape, producer-side correctness, regressions to dealbot operations); do not propose ClickHouse schema, retention, or operational tuning changes. +- A "check" is a task type (data storage, retrieval, data retention). A "job" is one execution of a check against one SP. + +## Core Invariants + +- At most one job per SP per check type per network runs at a time. Flag changes that could allow concurrent or duplicate jobs for the same SP on the same network. +- A job is marked failed only when job execution itself fails. A check that completes and reports a negative outcome is a successful job with a failing check result. Do not treat a negative check result as a job failure. +- Scheduling, cleanup, active-provider filtering, and queue execution must agree on the same set of SPs and the same network. +- For network-aware code, inserts, queries, conflict keys, filters, and tests must all carry the same network invariants. + +## Priorities + +### Blocker + +- Security vulnerabilities, secrets exposure, broken auth/authz, unsafe input handling, or logs that could leak tokens, credentials, or private data. +- Correctness defects that break job lifecycle invariants, provider filtering, multi-network behavior, retries, queue execution, or state transitions. +- Breaking schema, migration, query, config, or storage changes without compatibility, rollout, or migration safety. +- Missing validation, permission checks, or failure handling where users, data, or production stability could be harmed. + +### Important + +- Tests, mocks, or fixtures that no longer match the real runtime or API contract. +- SQL issues involving quoted identifiers, nullability, conflict keys, cross-network collisions, or behavior differences between databases. +- Error handling or observability gaps that would make failures hard to diagnose, mitigate, or roll back. +- Performance or resource issues likely to matter in normal production use, not hypothetical micro-optimizations. + +## Repository-Specific Focus + +- When changing provider filtering or blocklists, look for stale schedules, skipped cleanup, duplicate work, or drift between selection and execution paths. +- For frontend changes, prioritize user-visible correctness and contract drift over low-impact optimizations. + +## Do Not Comment On + +- Formatting, lint, import order, generated files, lockfiles, or issues already enforced by CI, formatters, or static analysis (unless the issue reflects a real runtime bug or missing invariant). +- Pure preference between multiple reasonable approaches. +- Minor naming tweaks, comment additions, or speculative future refactors unless they hide a real defect or maintenance risk. + +## Comment Format + +Use one issue per comment: + +- `Severity: short title` +- what is wrong and where it appears +- why it matters +- minimal fix or concrete next step From 51f6d24812e83629dcfdf3a160faf847ac613dd3 Mon Sep 17 00:00:00 2001 From: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com> Date: Tue, 28 Apr 2026 10:27:45 -0400 Subject: [PATCH 2/2] chore: address Copilot feedback on review instructions - Clarify ClickHouse ownership: DDL (clickhouse.schema.ts) and event payloads are owned in-repo and in-scope for review; only cluster ops/retention/infra tuning are externally owned. Earlier wording could have suppressed legitimate schema review (Copilot caught this). - Add Prometheus as a source of truth alongside Postgres, and discourage adding new persisted DB state without need. - Align Comment Format header with the Blocker/Important priority scheme instead of a free-form `Severity:` label. - Drop the generic performance Important bullet; not backed by the PR-comment evidence and frees bytes for the above. Final size: 3950/4000 bytes. --- .github/copilot-instructions.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 8f868773..fe46a25f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -16,8 +16,8 @@ Review for material correctness, security, state-consistency, compatibility, or ## Dealbot Context - TypeScript monorepo: NestJS backend (`apps/backend`), React/Vite frontend (`apps/web`), subgraph (`apps/subgraph`). -- Postgres is the source of truth for operational state. -- ClickHouse stores telemetry events. The cluster, schema, and migrations are owned by an external team. Review ClickHouse-touching code for dealbot impact (event payload shape, producer-side correctness, regressions to dealbot operations); do not propose ClickHouse schema, retention, or operational tuning changes. +- Sources of truth: Postgres (operational state) and Prometheus (metrics). Discourage adding new persisted DB state; prefer events/metrics or reusing existing columns. Flag PRs that store data the system does not need. +- ClickHouse: the DDL (`apps/backend/src/clickhouse/clickhouse.schema.ts`) and event payloads are owned in-repo and in-scope for review (schema, column types, payload shape, producer correctness). Cluster ops, retention, and infra tuning are owned by an external team — do not propose changes there. - A "check" is a task type (data storage, retrieval, data retention). A "job" is one execution of a check against one SP. ## Core Invariants @@ -41,7 +41,6 @@ Review for material correctness, security, state-consistency, compatibility, or - Tests, mocks, or fixtures that no longer match the real runtime or API contract. - SQL issues involving quoted identifiers, nullability, conflict keys, cross-network collisions, or behavior differences between databases. - Error handling or observability gaps that would make failures hard to diagnose, mitigate, or roll back. -- Performance or resource issues likely to matter in normal production use, not hypothetical micro-optimizations. ## Repository-Specific Focus @@ -58,7 +57,7 @@ Review for material correctness, security, state-consistency, compatibility, or Use one issue per comment: -- `Severity: short title` +- `Blocker: short title` or `Important: short title` - what is wrong and where it appears - why it matters - minimal fix or concrete next step