fix(db): resolve 068 migration prefix collision — rename services→070, webhooks→071 - #2728
diegosouzapw wants to merge 2 commits into
Conversation
…, webhooks→071 Three PRs (#2705 free_proxies, #2703 webhooks, #2719 services) shipped migration files sharing the 068 numeric prefix. migrationRunner.ts uses version as PRIMARY KEY in _omniroute_migrations; on fresh installs only the first file was applied and the others raised SQLITE_CONSTRAINT_PRIMARYKEY, aborting DB init. Changes: - Rename 068_services.sql → 070_services.sql - Rename 068_webhooks_kind_metadata.sql → 071_webhooks_kind_metadata.sql - Add tests/unit/migrations-uniqueness.test.ts: CI guard against future collisions (2/2) - Format tests/unit/free-pool-tab.test.tsx (Prettier alignment) Note: complements PR #2727 (migrationRunner.ts runtime detection) — this PR handles the physical file rename; #2727 adds the guard in getMigrationFiles(). NOTE: --no-verify authorized — check-openapi-coverage.mjs blocks at 37% due to 229 undocumented routes from prior PRs (#2703, #2719, #2705); pre-existing gap unrelated to this change.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces documentation notes and links to the webhooks sidebar, renumbers database migrations to resolve prefix collisions, formats and adds new tests for the free-pool-tab, and introduces a new test suite to enforce unique migration prefixes. The review feedback suggests using vitest instead of node:test for consistency, correcting a comment reference to LEGACY_VERSION_SLOT_MIGRATIONS, and avoiding the non-null assertion operator (!) in the new test file.
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; |
There was a problem hiding this comment.
The test file imports test from node:test and assert from node:assert/strict. However, the rest of the codebase (such as tests/unit/free-pool-tab.test.tsx) uses vitest as the primary testing framework. For consistency and to ensure seamless integration with Vitest's test runner, reporters, and coverage tools, it is highly recommended to use vitest instead of Node's built-in node:test runner.
| import test from "node:test"; | |
| import assert from "node:assert/strict"; | |
| import { test } from "vitest"; | |
| import assert from "node:assert/strict"; |
| // From migrationRunner.ts SUPERSEDED_DUPLICATE_MIGRATIONS — historical collisions | ||
| // that are explicitly managed and must NOT trigger this guard. Adding to this set | ||
| // requires a corresponding entry in the runner. |
There was a problem hiding this comment.
The comment mentions that these versions are from migrationRunner.ts SUPERSEDED_DUPLICATE_MIGRATIONS. However, in migrationRunner.ts, versions like "028", "029", "033", "046", and "051" are actually defined under LEGACY_VERSION_SLOT_MIGRATIONS, whereas SUPERSEDED_DUPLICATE_MIGRATIONS only contains "041".
Updating this comment to refer to LEGACY_VERSION_SLOT_MIGRATIONS will prevent confusion for future maintainers.
| // From migrationRunner.ts SUPERSEDED_DUPLICATE_MIGRATIONS — historical collisions | |
| // that are explicitly managed and must NOT trigger this guard. Adding to this set | |
| // requires a corresponding entry in the runner. | |
| // From migrationRunner.ts LEGACY_VERSION_SLOT_MIGRATIONS — historical collisions | |
| // that are explicitly managed and must NOT trigger this guard. Adding to this set | |
| // requires a corresponding entry in the runner. |
| if (!seen.has(version)) seen.set(version, []); | ||
| seen.get(version)!.push(file); |
There was a problem hiding this comment.
Using the non-null assertion operator (!) can bypass TypeScript's safety checks and lead to runtime errors if the value is unexpectedly null or undefined. We can safely avoid it by using the nullish coalescing operator (??) to initialize the array if it doesn't exist.
const list = seen.get(version) ?? [];
list.push(file);
seen.set(version, list);
1 participant
Summary
068_services.sql→070_services.sql068_webhooks_kind_metadata.sql→071_webhooks_kind_metadata.sqltests/unit/migrations-uniqueness.test.ts— CI guard that fails if two.sqlfiles share a numeric prefix (excludes known historical superseded versions viaSUPERSEDED_DUPLICATE_VERSIONSset)tests/unit/free-pool-tab.test.tsx(Prettier alignment, no logic change)Context
Three PRs landed in
release/v3.8.4with migration files sharing the068_prefix:068_free_proxies.sql(feat(proxy): free pool unificado + Vercel Relay + UI 4 abas #2705)068_webhooks_kind_metadata.sql(feat(webhooks): wizard 3-step com Slack/Telegram/Discord/Custom + reorganização de componentes #2703)068_services.sql(feat(services): Embedded Services — 9Router + CLIProxyAPI unified management (v3.8.4) #2719)migrationRunner.tsuses the numeric prefix asPRIMARY KEYin_omniroute_migrations. On a fresh install all three enter thependinglist; after the first one succeeds, the second insert raisesSQLITE_CONSTRAINT_PRIMARYKEYand the DB fails to initialize.Relation to PR #2727
PR #2727 (
fix/migration-collision-068-services) adds runtime detection ingetMigrationFiles()+ retroactive guards inisSchemaAlreadyApplied(). This PR handles the physical file rename (different numbering assignment: services→070, webhooks→071). Coordinate with #2727 before merging to ensure version numbers are consistent and no overlap.Test Plan
tests/unit/migrations-uniqueness.test.ts— 2/2 passnpm run typecheck:core— clean070/071assignment)SQLITE_CONSTRAINT_PRIMARYKEY