feat: add retrieval_type column to retrieval_checks clickhouse table by iand · Pull Request #485 · FilOzone/dealbot

iand · 2026-04-28T08:42:38Z

Adds a retrieval_type column to the clickhouse retrieval_checks table as suggested in #438 (comment)

This column is populated with the value deal by the current deal-led retrieval check. Other retrieval checks can add other values.

Used this change to incorporate a simple schema migration system for clickhouse similar to the one used for postgres. This adds another table schema_migrations to track which migrations have been applied and what version the schema is at.

Copilot

Pull request overview

Adds a retrieval_type dimension to ClickHouse retrieval_checks rows and introduces a simple ClickHouse schema migration mechanism (tracked via a schema_migrations table) so future schema evolution can be versioned and applied on startup.

Changes:

Writes retrieval_type: "deal" for deal-led retrieval checks when inserting into ClickHouse.
Replaces the prior “run DDL on startup” approach with versioned ClickHouse migrations tracked in a schema_migrations table.
Adds a ClickHouse migration to add the retrieval_type column to retrieval_checks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
apps/backend/src/retrieval/retrieval.service.ts	Adds `retrieval_type: "deal"` to ClickHouse inserts for retrieval checks.
apps/backend/src/clickhouse/clickhouse.service.ts	Implements migration application + rollback tracking via `schema_migrations`.
apps/backend/src/clickhouse/clickhouse.migrations.ts	Defines versioned migrations (initial schema + add `retrieval_type` column).

Comments suppressed due to low confidence (1)

apps/backend/src/clickhouse/clickhouse.migrations.ts:101

Migration add_retrieval_type_to_retrieval_checks adds a non-null retrieval_type column without a DEFAULT or backfill. Existing rows will get the type default (empty string), so historical data won’t be labeled as deal and queries filtering on retrieval_type='deal' will miss old records. Consider adding the column with DEFAULT 'deal' (and optionally backfilling existing rows) since all existing writes appear to be deal-led retrieval checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iand · 2026-04-28T08:54:04Z

    }
-    this.logger.log({ event: "clickhouse_migrated", database });
+    await this.client.command({
+      query: `ALTER TABLE ${this.database}.schema_migrations DELETE WHERE version = ${version}`,


These points are both reasonable and I'll change.

iand · 2026-04-28T08:55:45Z

    const parsedUrl = new URL(this.config.url);
-    const database = parsedUrl.pathname.replace(/^\//, "");
+    this.database = parsedUrl.pathname.replace(/^\//, "");
    try {
-      await this.migrate(database);
+      await this.migrate(this.database);


Again, this is valid and tightens the code against deployment errors

iand · 2026-04-28T08:58:09Z

+    await this.client.command({
+      query: `CREATE TABLE IF NOT EXISTS ${database}.schema_migrations
+(
+    version    UInt32,


Adding some stricter validation on the CLICKHOUSE_URL

iand · 2026-04-28T09:08:54Z

+      for (const sql of m.up) {
+        await this.client.command({ query: sql });
+      }
+      await this.client.insert({
+        table: `${database}.schema_migrations`,
+        values: [{ version: m.version, name: m.name }],
+        format: "JSONEachRow",
+      });
+      this.logger.log({ event: "migration_applied", version: m.version, name: m.name });
+      count++;
+    }
+
+    this.logger.log({ event: "clickhouse_migrated", database, appliedCount: count });
+  }
+


This is another valid point (though the code copilot suggests uses raw interpolation which it complained about in an earlier comment!). It does make the migration code slightly more complex but it will be more robust when we run in multiple locations

This suggested code it not great. It loops through migrations and if it can't obtain a lock for migration N it continues and attempts to get a lock for N+1, which may depend on the migration N. Plus whatever process held the lock for migration N now can't obtain the next one it needs.

Copilot

Pull request overview

Adds a retrieval_type dimension to ClickHouse retrieval_checks and introduces a simple ClickHouse schema migration system (tracked via a schema_migrations table) so future schema changes can be applied incrementally and safely.

Changes:

Adds retrieval_type: "deal" to the deal-led retrieval check row inserted into retrieval_checks.
Refactors ClickHouse startup DDL into versioned migrations with a schema_migrations tracking table and a startup migration lock.
Adds a migration to ALTER TABLE ... ADD COLUMN retrieval_type on retrieval_checks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
apps/backend/src/retrieval/retrieval.service.ts	Adds `retrieval_type` to the ClickHouse row emitted for retrieval checks.
apps/backend/src/clickhouse/clickhouse.service.ts	Implements migration tracking + locking, validates DB name parsed from `CLICKHOUSE_URL`, and runs migrations on startup.
apps/backend/src/clickhouse/clickhouse.migrations.ts	Defines versioned migrations: initial table DDL plus `retrieval_type` column addition.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iand · 2026-04-28T09:41:14Z

+      const migrations = getMigrations(database);
+      let count = 0;
+      let schemaVersion = applied.size > 0 ? Math.max(...applied) : 0;
+      for (const m of migrations) {
+        if (applied.has(m.version)) continue;
+        for (const sql of m.up) {
+          await this.client.command({ query: sql });


Sorting the migrations is cheap and seems sensible to avoid errors down the road

iand · 2026-04-28T09:43:22Z

+    await this.client.command({
+      query: `CREATE TABLE IF NOT EXISTS ${database}.schema_migrations
+(
+    version    UInt32,
+    name       String,
+    applied_at DateTime64(3, 'UTC') DEFAULT now64()
+)
+ENGINE = MergeTree()
+ORDER BY version`,
+    });


This doesn't seem worth the effort. The database is a required precondition for dealbot when DEALBASE_URL is set, and the first migration error will cause dealbot to refuse to start if the database is not accessible on startup

as agreed previously, since probelab is creating the clickhouse DB, i'm good with your reasoning here.

iand · 2026-04-28T09:45:31Z

+      this.logger.error({ event: "migration_locked", message: "Another instance is running migrations" });
+      throw new Error("Migration lock is held by another instance");


This is already noted in a comment. This situation only happens if dealbot crashes while holding the lock. Adding it to the log could be helpful though.

the copilot suggested code hardcodes the name of the lock table

Copilot

Pull request overview

Adds a retrieval_type dimension to ClickHouse retrieval_checks and introduces a versioned ClickHouse schema migration mechanism (tracked in schema_migrations) to evolve the ClickHouse schema safely over time.

Changes:

Populate a new retrieval_type field ("deal") when writing retrieval_checks rows.
Replace startup “create-if-not-exists” DDL execution with a versioned migration runner + schema_migrations tracking and a migration lock table.
Add migration v2 to add/drop the retrieval_type column on retrieval_checks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
apps/backend/src/retrieval/retrieval.service.ts	Adds `retrieval_type: "deal"` to ClickHouse inserts for retrieval checks.
apps/backend/src/clickhouse/clickhouse.service.ts	Implements ClickHouse migration runner, schema version tracking table, and migration lock; updates logging and URL DB parsing.
apps/backend/src/clickhouse/clickhouse.migrations.ts	Defines versioned migrations including initial schema and the new `retrieval_type` column migration.

Comments suppressed due to low confidence (1)

apps/backend/src/clickhouse/clickhouse.migrations.ts:12

Repository docs still refer to apps/backend/src/clickhouse/clickhouse.schema.ts as the source of truth for ClickHouse DDL, but this PR has the DDL in clickhouse.migrations.ts and that clickhouse.schema.ts path appears to be gone. Please update the docs references (e.g. docs/checks/events-and-metrics.md) to avoid broken links/confusion about where the authoritative schema lives.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iand · 2026-04-28T10:03:52Z

    }
-    this.logger.log({ event: "clickhouse_migrated", database });
+    await this.client.command({
+      query: `DELETE FROM ${this.database}.schema_migrations WHERE version = {version:UInt32}`,


DELETE FROM in clickhouse marks rows as deleted immediately via a deletion mask so this isn't needed

iand · 2026-04-28T10:04:02Z

+  async migrateDown(version: number): Promise<void> {
+    if (!this.client) throw new Error("ClickHouse not connected");
+    const migrations = getMigrations(this.database);
+    const migration = migrations.find((m) => m.version === version);
+    if (!migration) throw new Error(`Migration version ${version} not found`);
+
+    for (const sql of migration.down) {
      await this.client.command({ query: sql });
    }
-    this.logger.log({ event: "clickhouse_migrated", database });
+    await this.client.command({
+      query: `DELETE FROM ${this.database}.schema_migrations WHERE version = {version:UInt32}`,
+      query_params: { version },
+    });
+    this.logger.log({ event: "migration_rolled_back", version, name: migration.name });
  }


Copilot

Pull request overview

Adds a retrieval_type dimension to ClickHouse retrieval_checks records and introduces a lightweight ClickHouse schema migration mechanism (tracked via a schema_migrations table) so schema changes can be applied incrementally on startup.

Changes:

Add retrieval_type field to ClickHouse writes for deal-led retrieval checks.
Introduce ClickHouse schema versioning via schema_migrations + a migration lock table.
Convert ClickHouse DDL bootstrapping into versioned up/down migrations, including a migration to add retrieval_type.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
apps/backend/src/retrieval/retrieval.service.ts	Writes `retrieval_type: "deal"` into `retrieval_checks` rows.
apps/backend/src/clickhouse/clickhouse.service.ts	Adds migration runner with schema tracking, locking, and `migrateDown()`.
apps/backend/src/clickhouse/clickhouse.migrations.ts	Defines versioned migrations (initial schema + add `retrieval_type`).

Comments suppressed due to low confidence (1)

apps/backend/src/clickhouse/clickhouse.migrations.ts:101

Migration v2 adds retrieval_type without a DEFAULT/backfill. Existing rows in retrieval_checks (written before this column existed) will read back as the type’s default value (empty string), which makes historical data inconsistent with the new semantics where all deal-led retrievals are deal. Consider adding a DEFAULT (e.g., 'deal') in the ADD COLUMN so old rows automatically present as deal, or add an explicit backfill mutation in the migration if you need a different default strategy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

iand · 2026-04-28T10:13:25Z

+  async migrateDown(version: number): Promise<void> {
+    if (!this.client) throw new Error("ClickHouse not connected");
+    const migrations = getMigrations(this.database);
+    const migration = migrations.find((m) => m.version === version);
+    if (!migration) throw new Error(`Migration version ${version} not found`);
+
+    const lockAcquired = await this.tryAcquireMigrationLock(this.database);
+    if (!lockAcquired) {
+      const lockTable = this.migrationLockTable(this.database);
+      throw new Error(`Could not acquire migration lock on ${lockTable}. Another migration operation is in progress.`);
+    }


I don't think this is worth the effort

iand · 2026-04-30T08:10:43Z

It seems that this new column is not needed since #487 uses its own extended retrieval table. It leaves the retrieval_checks as the table used for retrievals associated with a deal made by dealbot.

Given that, I will close this PR. We can always resurrect it if we want to add the migration capability separately.

* 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. * 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.

iand added 2 commits April 28, 2026 09:12

feat: add retrieval_type column to retrieval_checks clickhouse table

76a3b76

feat: add schema migration system for clickhouse

eeab833

Copilot AI review requested due to automatic review settings April 28, 2026 08:42

FilOzzy added this to FOC Apr 28, 2026

github-project-automation Bot moved this to 📌 Triage in FOC Apr 28, 2026

Copilot started reviewing on behalf of iand April 28, 2026 08:43 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

fix: take a global lock while running migrations

7f06732

iand requested a review from Copilot April 28, 2026 09:32

Copilot started reviewing on behalf of iand April 28, 2026 09:32 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

fix: sort list of migrations and improve lock error message

7eb3021

iand requested a review from Copilot April 28, 2026 09:49

Copilot started reviewing on behalf of iand April 28, 2026 09:49 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

fix: wrap migrateDown in global lock

08972c6

iand requested a review from Copilot April 28, 2026 10:05

Copilot started reviewing on behalf of iand April 28, 2026 10:06 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

SgtPooki assigned iand Apr 28, 2026

SgtPooki mentioned this pull request Apr 28, 2026

chore: add Copilot PR review instructions #489

Merged

4 tasks

BigLep moved this from 📌 Triage to 🔎 Awaiting review in FOC Apr 28, 2026

dennis-tra mentioned this pull request Apr 29, 2026

feat: anon piece selection and retrieval #487

Open

SgtPooki closed this Apr 30, 2026

github-project-automation Bot moved this from 🔎 Awaiting review to 🎉 Done in FOC Apr 30, 2026

iand mentioned this pull request May 11, 2026

feat: add pull_checks table to clickhouse #522

Merged

		this.logger.error({ event: "migration_locked", message: "Another instance is running migrations" });
		throw new Error("Migration lock is held by another instance");

Conversation

iand commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iand Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iand commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iand Apr 28, 2026 •

edited

Loading

iand commented Apr 30, 2026 •

edited

Loading