Skip to content

PLAT-01: SQLite-to-PostgreSQL migration strategy, runbook, and compatibility harness#801

Merged
Chris0Jeky merged 8 commits intomainfrom
chore/sqlite-to-prod-db-migration-plan
Apr 12, 2026
Merged

PLAT-01: SQLite-to-PostgreSQL migration strategy, runbook, and compatibility harness#801
Chris0Jeky merged 8 commits intomainfrom
chore/sqlite-to-prod-db-migration-plan

Conversation

@Chris0Jeky
Copy link
Copy Markdown
Owner

Summary

  • ADR-0023: Documents PostgreSQL as the target production database provider. Covers alternatives analysis (SQL Server, CockroachDB, MySQL), migration approach via EF Core provider abstraction, and consequences. SQLite retained for local development and CI.
  • Migration runbook (docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md): Step-by-step guide covering pre-migration checklist, schema export via EF Core migrations, CSV-based data export/import in dependency order, row-count and foreign-key integrity verification, application smoke test checklist, rollback procedure, known provider differences table, FTS migration note, and security considerations for credential handling.
  • Provider compatibility test harness (DatabaseProviderCompatibilityTests.cs): 20 tests validating critical persistence operations across providers — CRUD on Board/Card/AutomationProposal, DateTimeOffset round-trip fidelity, GUID storage and FK joins, string collation, ordering and pagination, enum storage, aggregate queries, boolean filtering, concurrent inserts, Unicode string preservation. Documents SQLite's DateTimeOffset ORDER BY limitation as a known provider difference.
  • Docs updated: STATUS.md and IMPLEMENTATION_MASTERPLAN.md reflect delivery.

No domain-layer or application-layer changes. No new NuGet dependencies. All 20 new tests pass against SQLite.

Closes #84

Test plan

  • All 20 DatabaseProviderCompatibilityTests pass (dotnet test --filter "FullyQualifiedName~DatabaseProviderCompatibilityTests")
  • Full solution builds without errors (dotnet build backend/Taskdeck.sln -c Release)
  • ADR follows existing template and numbering convention (ADR-0023, added to INDEX.md)
  • Runbook covers rollback procedure and data integrity verification
  • CI pipeline passes

Document the decision to adopt PostgreSQL as the target production
database provider while retaining SQLite for local development and CI.
Covers alternatives analysis (SQL Server, CockroachDB, MySQL),
migration approach via EF Core provider abstraction, and consequences.

Refs #84
Step-by-step guide covering pre-migration checklist, schema export via
EF Core migrations, CSV-based data export/import in dependency order,
row-count and foreign-key integrity verification, smoke test checklist,
rollback procedure, and security considerations for credential handling.

Refs #84
Validates critical persistence operations across database providers
using EF Core's provider abstraction. Tests cover: CRUD on Board,
Card, and AutomationProposal; DateTimeOffset round-trip fidelity;
GUID storage and foreign-key joins; string collation behavior;
nullable field handling; ordering and pagination; enum storage and
filtering; aggregate queries (COUNT, GROUP BY); boolean filtering;
concurrent insert safety; Unicode string preservation; and a
documented test for SQLite's DateTimeOffset ORDER BY limitation.

All tests run against SQLite by default (CI); PostgreSQL opt-in via
TASKDECK_TEST_POSTGRES_CONNECTION environment variable (future).

Refs #84
Add delivery notes for SQLite-to-PostgreSQL migration strategy:
ADR-0023, migration runbook, and 20-test compatibility harness.

Refs #84
Copilot AI review requested due to automatic review settings April 9, 2026 02:30
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Adversarial Self-Review

Issues Found

1. MEDIUM — Runbook: Missing __EFMigrationsHistory table in row-count verification

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, Steps 2-4
  • EF Core creates __EFMigrationsHistory to track applied migrations. The row-count verification queries and CSV export do not include this table. After dotnet ef database update creates the schema in PostgreSQL, the migrations history is already populated — but the SQLite source may have a different set of applied migration entries. This is not a data table, but if the operator later runs dotnet ef database update against the migrated PostgreSQL instance, EF Core may try to re-apply migrations if the history doesn't match.
  • Fix: Add a note about __EFMigrationsHistory being managed by EF Core schema step, not data migration.

2. MEDIUM — Runbook: Missing ApiKeys table from row counts and CSV export

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, Steps 2-4
  • PR MCP-03: HTTP transport with API key authentication #792 added an ApiKeys table (MCP HTTP transport). The runbook's table lists do not include it. An operator following the runbook would lose all API keys during migration.
  • Fix: Add ApiKeys to the export, import, and verification query sets.

3. LOW — Runbook: PostgreSQL \COPY needs schema-qualified table names

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, Step 3
  • The \COPY command uses "${table}" but EF Core Npgsql may create tables in a non-default schema or with different casing. The correct quoted identifier should work for the default public schema, but a note about verifying the target schema would improve robustness.
  • Fix: Add schema verification step.

4. LOW — Runbook: Import script section header says "Disable triggers during import" but does not actually do it

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, Step 3
  • The import section says "Disable triggers during import for performance" but the script does not include ALTER TABLE ... DISABLE TRIGGER ALL. This is misleading.
  • Fix: Either add the trigger disable/enable commands or remove the suggestion.

5. LOW — Runbook: Missing NotificationPreferences and CommandRunLogs from PostgreSQL verification query

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, Step 4
  • The PostgreSQL verification query omits NotificationPreferences and CommandRunLogs even though they appear in the export step.
  • Fix: Add them to the verification query.

6. LOW — Test harness: String_CaseSensitiveComparison_IsConsistent name is misleading

  • File: backend/tests/Taskdeck.Api.Tests/DatabaseProviderCompatibilityTests.cs, line ~318
  • Test asserts exact == equality returns 1 match for "IMPORTANT TASK". However, SQLite's = operator for text is actually case-sensitive by default (unlike LIKE), while PostgreSQL's = is also case-sensitive. The test name implies it's documenting a difference, but both providers behave the same for =. The name should clarify it tests that equality is case-sensitive on both providers.
  • Fix: No behavioral fix needed, but a comment clarifying both providers agree would improve documentation value.

7. LOW — ADR: "checksum verification" mentioned in Decision but runbook only has row-count verification

  • File: docs/decisions/ADR-0023-sqlite-to-postgresql-migration-strategy.md, Decision section point 4
  • The ADR says "row-count and checksum verification" but the runbook only implements row-count verification. No checksum mechanism is provided.
  • Fix: Change the ADR to say "row-count and foreign-key integrity verification" to match the runbook's actual capabilities.

No Issues Found In

  • ADR alternatives analysis is thorough and accurate
  • ADR correctly references existing ADRs (0001, 0014) and issues (STRAT-00: Taskdeck platform expansion master tracker #531, CLD-00: Cloud and collaboration wave tracker #537)
  • Test harness does not introduce domain-layer coupling — all tests use EF Core abstractions
  • No credentials or secrets in any file
  • All test usernames/emails use unique prefixes (compat-*) avoiding collision with other test classes
  • INDEX.md entry correctly numbered ADR-0023
  • STATUS.md and IMPLEMENTATION_MASTERPLAN.md entries are factually accurate

Runbook:
- Add missing ApiKeys table to export, import, and verification steps
- Add __EFMigrationsHistory note (managed by schema step, not data)
- Add NotificationPreferences and CommandRunLogs to verification query
- Remove misleading "disable triggers" text from import section
- Add schema verification note for EF Core Npgsql table naming

ADR:
- Change "checksum verification" to "foreign-key integrity verification"
  to match the runbook's actual capabilities

Tests:
- Clarify case-sensitive comparison test comment (both providers agree)

Refs #84
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a documented strategy and operator runbook for migrating Taskdeck from SQLite to PostgreSQL, plus a new persistence compatibility test harness intended to guard cross-provider behavior as the platform moves toward hosted deployments.

Changes:

  • Added ADR-0023 documenting PostgreSQL as the target production DB and the planned migration/testing approach.
  • Added a SQLite→PostgreSQL migration runbook with export/import and verification steps.
  • Added DatabaseProviderCompatibilityTests to validate key persistence/query behaviors (currently wired to SQLite via existing test factory).

Reviewed changes

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

Show a summary per file
File Description
docs/STATUS.md Updates project status to reflect the migration strategy/harness deliverable.
docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md New operational runbook for schema + data migration and verification/rollback.
docs/IMPLEMENTATION_MASTERPLAN.md Records delivery of PLAT-01 items and notes future CI wiring for PostgreSQL tests.
docs/decisions/INDEX.md Registers ADR-0023 in the ADR index.
docs/decisions/ADR-0023-sqlite-to-postgresql-migration-strategy.md New ADR describing provider decision, approach, and consequences.
backend/tests/Taskdeck.Api.Tests/DatabaseProviderCompatibilityTests.cs New compatibility test harness for critical persistence behaviors.

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

Comment on lines +59 to +62
**Note**: The `__EFMigrationsHistory` table tracks applied EF Core migrations. It is populated automatically by `dotnet ef database update` in Step 1 and should **not** be exported or imported as data — the schema step handles it.

4. **Provision the PostgreSQL database**:
```bash
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provisioning section grants ALL PRIVILEGES ON DATABASE taskdeck to the application user, but the later “Security Considerations” section recommends restricting permissions. Consider tightening the runbook to grant only what the app needs (CONNECT + schema USAGE + DML on tables), or explicitly label the GRANT ALL PRIVILEGES step as a temporary bootstrap that should be replaced by least-privilege grants.

Copilot uses AI. Check for mistakes.
docs/STATUS.md Outdated
- **Property-based and adversarial input tests** (`#717`/`#789`): 211 tests across 5 files — 77 FsCheck domain entity tests (adversarial strings: unicode, null bytes, BOM, ZWSP, RTL override, surrogate pairs, XSS, SQL injection; boundary lengths; GUID/position validation), 29 JSON serialization round-trip fuzz tests (GUID format variations, DateTime boundaries, malformed JSON, large payloads), 80 API adversarial integration tests (no 500s from any adversarial input across board/card/column/capture/auth/search endpoints, malformed JSON, wrong content types, concurrent adversarial requests), 16 fast-check frontend input sanitization property tests, 9 store resilience property tests; `fast-check` added as frontend dev dependency; adversarial review fixed capture payload round-trip testing wrong DTO and null handling inconsistency in FsCheck generators
- **Inbox premium primitives** (`#249`/`#788`): `InboxView.vue` reworked to use shared UI primitive components — `TdSkeleton` for loading states, `TdInlineAlert` for errors, `TdEmptyState` for empty list, `TdBadge` for status chips, `TdSpinner` for detail refresh; ~65 lines of redundant CSS removed; 7 new vitest tests; adversarial review fixed skeleton screen reader announcements (added `role="status"` and sr-only labels) and redundant `role="alert"` nesting

- SQLite-to-PostgreSQL production migration strategy delivered (`#84`): ADR-0023 documents PostgreSQL as the target production provider (SQLite retained for local/CI); migration runbook at `docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md` covers schema export, data migration, integrity verification, rollback, and security; 20-test provider-compatibility harness in `DatabaseProviderCompatibilityTests` validates CRUD, date/time fidelity, GUID handling, string collation, ordering, pagination, enum storage, aggregates, boolean filtering, concurrent inserts, and Unicode across providers; documents SQLite's `DateTimeOffset` ORDER BY limitation as a known provider difference
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provider-compatibility harness is described here as validating behavior “across providers”, but the current TestWebApplicationFactory hard-wires SQLite and there is no provider-switching path in this PR. Please adjust this status entry to reflect the current reality (SQLite-only today), or implement/land the opt-in PostgreSQL factory + test run and then keep this wording.

Copilot uses AI. Check for mistakes.
Comment on lines +623 to +627
130. SQLite-to-PostgreSQL production migration strategy (`#84`, 2026-04-09):
- ADR-0023: recommends PostgreSQL as target production DB provider; documents alternatives (SQL Server, CockroachDB, MySQL) and tradeoffs; SQLite retained for local dev and CI
- migration runbook at `docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md`: schema export via EF Core migrations, CSV-based data export/import in dependency order, row-count and foreign-key integrity verification, smoke test checklist, rollback procedure, security considerations
- 20-test provider-compatibility harness (`DatabaseProviderCompatibilityTests`): CRUD on Board/Card/Proposal, DateTimeOffset fidelity, GUID storage and FK joins, string collation, ordering, pagination, enum storage, aggregates, boolean filtering, concurrent inserts, Unicode; documents SQLite `DateTimeOffset` ORDER BY limitation
- PostgreSQL tests opt-in via `TASKDECK_TEST_POSTGRES_CONNECTION` environment variable (future CI integration)
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entry claims PostgreSQL runs are opt-in via TASKDECK_TEST_POSTGRES_CONNECTION, but no such switch exists in the test infrastructure in this PR (the test factory uses SQLite unconditionally). Please reword as “planned/next step” or add the actual opt-in PostgreSQL test wiring before documenting it here.

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +31
1. **Provider abstraction via EF Core**: The application already uses EF Core with repository interfaces defined in the Application layer. No domain-layer changes are required. The Infrastructure layer's `DependencyInjection.AddInfrastructure()` will gain a configuration switch (`DatabaseProvider`) to select between `Sqlite` and `PostgreSQL`.

2. **Dual-provider compatibility testing**: A test harness validates that critical persistence operations (CRUD, queries, date/time handling, string collation, GUID storage) produce consistent results across SQLite and PostgreSQL. Tests run against SQLite in CI by default; PostgreSQL tests are opt-in via environment variable (`TASKDECK_TEST_POSTGRES_CONNECTION`).

3. **Schema migration**: EF Core migrations remain the source of truth. A PostgreSQL migration bundle will be generated from the same model. SQLite-specific constructs (e.g., `AUTOINCREMENT`, FTS5 virtual tables) will need provider-conditional handling.
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ADR states PostgreSQL compatibility tests are opt-in via TASKDECK_TEST_POSTGRES_CONNECTION, but there’s no implementation of that environment-variable switch/provider-switching factory in the repo right now (SQLite is hard-coded in TestWebApplicationFactory). Please mark this as a future follow-up (and describe the intended mechanism), or implement the switch before asserting it as part of the decision.

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +76

Configure the application to target PostgreSQL and apply migrations.

```bash
# Set the connection string for PostgreSQL
export ConnectionStrings__DefaultConnection="Host=<host>;Database=taskdeck;Username=taskdeck_app;Password=<password>"
export Taskdeck__DatabaseProvider="PostgreSQL"

Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 1 assumes the app can be switched to PostgreSQL via Taskdeck__DatabaseProvider and that EF migrations will apply cleanly. In the current codebase there’s no DatabaseProvider config switch, and at least one migration contains SQLite-only SQL (FTS5 CREATE VIRTUAL TABLE / trigger), so dotnet ef database update against PostgreSQL will fail. Please either mark this runbook as requiring follow-up work (provider-conditional migrations + DI switch), or document the extra steps needed to make migrations PostgreSQL-compatible.

Copilot uses AI. Check for mistakes.
Comment on lines +12 to +18
/// <summary>
/// Provider-compatibility test harness validating that critical persistence
/// operations produce consistent results across database providers.
///
/// By default, tests run against SQLite (the CI default). When the environment
/// variable TASKDECK_TEST_POSTGRES_CONNECTION is set, a parallel PostgreSQL
/// run can be configured via a provider-switching factory.
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class summary references TASKDECK_TEST_POSTGRES_CONNECTION and a “provider-switching factory”, but the current TestWebApplicationFactory configures SQLite unconditionally and there’s no environment-variable switch implemented. Please update the docstring to match current behavior, or add the PostgreSQL opt-in wiring so the harness can actually run against both providers.

Copilot uses AI. Check for mistakes.
Comment on lines +342 to +375

[Fact]
public async Task String_ContainsQuery_BehaviorIsConsistent()
{
using var scope = _factory.Services.CreateScope();
var db = scope.ServiceProvider.GetRequiredService<TaskdeckDbContext>();

var user = new User("compat-contains", "compat-contains@example.com", "hash");
db.Users.Add(user);
var board = new Board("Contains Board", ownerId: user.Id);
db.Boards.Add(board);
var column = new Column(board.Id, "Col", 0);
db.Columns.Add(column);
await db.SaveChangesAsync();

var card1 = new Card(board.Id, column.Id, "Fix login bug", position: 0);
var card2 = new Card(board.Id, column.Id, "Add LOGIN feature", position: 1);
var card3 = new Card(board.Id, column.Id, "Update docs", position: 2);
db.Cards.AddRange(card1, card2, card3);
await db.SaveChangesAsync();

db.ChangeTracker.Clear();

// EF Core LIKE translation: Contains maps to SQL LIKE '%value%'
// SQLite LIKE is case-insensitive for ASCII by default
// PostgreSQL LIKE is case-sensitive; ILIKE is case-insensitive
// This test documents the SQLite baseline behavior
var containsLogin = await db.Cards
.Where(c => c.BoardId == board.Id && c.Title.Contains("login"))
.ToListAsync();

// SQLite LIKE is case-insensitive for ASCII, so both match
containsLogin.Count.Should().BeGreaterThanOrEqualTo(1,
"at least the lowercase 'login' card should match");
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String_ContainsQuery_BehaviorIsConsistent doesn’t assert consistent cross-provider behavior: it only checks Count >= 1, which will pass even if SQLite matches both “login” and “LOGIN” while PostgreSQL matches only one (case-sensitive LIKE). Please either rename/reframe this as documenting a known provider difference, or add provider-aware expectations so the harness actually detects incompatibilities.

Copilot uses AI. Check for mistakes.
Comment on lines +749 to +771

[Fact]
public async Task ConcurrentInserts_DoNotLoseData()
{
using var scope = _factory.Services.CreateScope();
var db = scope.ServiceProvider.GetRequiredService<TaskdeckDbContext>();

var user = new User("compat-concurrent", "compat-concurrent@example.com", "hash");
db.Users.Add(user);
var board = new Board("Concurrent Board", ownerId: user.Id);
db.Boards.Add(board);
var column = new Column(board.Id, "Col", 0);
db.Columns.Add(column);
await db.SaveChangesAsync();

// Insert 20 cards sequentially (SQLite doesn't support true concurrent writes)
var cardIds = new List<Guid>();
for (int i = 0; i < 20; i++)
{
var card = new Card(board.Id, column.Id, $"Concurrent Card {i}", position: i);
cardIds.Add(card.Id);
db.Cards.Add(card);
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConcurrentInserts_DoNotLoseData is labeled as a concurrent-write test, but it performs only sequential Add calls followed by a single SaveChangesAsync() (no parallelism, no multiple DbContexts/transactions). If you want to validate concurrency semantics on PostgreSQL, implement true concurrency behind a PostgreSQL-only gate; otherwise rename this test to reflect that it’s a bulk-insert persistence sanity check.

Copilot uses AI. Check for mistakes.
Comment on lines +189 to +195
**GUID column handling**: SQLite stores GUIDs as text strings. PostgreSQL with Npgsql maps `Guid` properties to the native `uuid` type. EF Core's Npgsql provider accepts standard UUID text format (`xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx`), so CSV import should work directly. If you encounter format errors, verify the GUID format in the CSV matches PostgreSQL's expected input.

**DateTimeOffset handling**: SQLite stores `DateTimeOffset` as ISO 8601 text strings. PostgreSQL stores them as `timestamptz`. The CSV import should parse ISO 8601 strings correctly. If timezone information is missing from SQLite values, PostgreSQL will assume UTC.

## Step 4: Verify Data Integrity

Run row-count verification against PostgreSQL and compare with the pre-migration counts from the checklist:
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PostgreSQL row-count verification query also omits several migrated tables (e.g., CardCommentMentions, CommandRunLogs, NotificationPreferences, AgentRunEvents, KnowledgeChunks). Please include the full set so the post-import verification actually covers everything you exported/imported.

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +34
3. **Schema migration**: EF Core migrations remain the source of truth. A PostgreSQL migration bundle will be generated from the same model. SQLite-specific constructs (e.g., `AUTOINCREMENT`, FTS5 virtual tables) will need provider-conditional handling.

4. **Data migration**: A documented runbook covers one-time data export from SQLite and import into PostgreSQL, with row-count and foreign-key integrity verification.

Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ADR states data migration will use “row-count and checksum verification”, but the runbook currently documents row counts + FK integrity checks only (no checksum procedure). Please align the ADR with the runbook (remove checksum mention) or add a concrete checksum verification step to the runbook so the stated verification approach is achievable.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a production migration strategy from SQLite to PostgreSQL, featuring ADR-0023, a comprehensive migration runbook, and a 20-test provider-compatibility harness. The reviewer identified several areas for improvement: normalizing string containment queries to handle provider-specific case-sensitivity, refactoring a concurrency test that currently executes sequentially, and including missing tables in the migration integrity checklists.

Comment on lines +373 to +374
// SQLite LIKE is case-insensitive for ASCII, so both match
containsLogin.Count.Should().BeGreaterThanOrEqualTo(1,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The assertion BeGreaterThanOrEqualTo(1) is too permissive for a compatibility test. SQLite's LIKE (used by EF Contains) is case-insensitive for ASCII by default, while PostgreSQL's is case-sensitive. This means the query returns different results depending on the provider (2 vs 1 in this test case). To ensure consistent application behavior across providers, the query should be normalized (e.g., using .ToUpper()) and the test should assert an exact match count.


// ─── Concurrent writes (basic safety) ───────────────────────────

[Fact]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test ConcurrentInserts_DoNotLoseData does not actually test concurrency. It performs sequential additions to a single DbContext followed by a single SaveChangesAsync call. To validate provider behavior under true concurrent load (multiple simultaneous writers), the test should use multiple DbContext instances across parallel tasks. If the intent is only to test batch inserts, the test should be renamed to avoid confusion.

Comment on lines +30 to +54
SELECT 'Users' AS tbl, COUNT(*) FROM Users
UNION ALL SELECT 'Boards', COUNT(*) FROM Boards
UNION ALL SELECT 'Columns', COUNT(*) FROM Columns
UNION ALL SELECT 'Cards', COUNT(*) FROM Cards
UNION ALL SELECT 'Labels', COUNT(*) FROM Labels
UNION ALL SELECT 'CardLabels', COUNT(*) FROM CardLabels
UNION ALL SELECT 'CardComments', COUNT(*) FROM CardComments
UNION ALL SELECT 'BoardAccesses', COUNT(*) FROM BoardAccesses
UNION ALL SELECT 'AuditLogs', COUNT(*) FROM AuditLogs
UNION ALL SELECT 'AutomationProposals', COUNT(*) FROM AutomationProposals
UNION ALL SELECT 'AutomationProposalOperations', COUNT(*) FROM AutomationProposalOperations
UNION ALL SELECT 'ArchiveItems', COUNT(*) FROM ArchiveItems
UNION ALL SELECT 'ChatSessions', COUNT(*) FROM ChatSessions
UNION ALL SELECT 'ChatMessages', COUNT(*) FROM ChatMessages
UNION ALL SELECT 'CommandRuns', COUNT(*) FROM CommandRuns
UNION ALL SELECT 'Notifications', COUNT(*) FROM Notifications
UNION ALL SELECT 'UserPreferences', COUNT(*) FROM UserPreferences
UNION ALL SELECT 'LlmRequests', COUNT(*) FROM LlmRequests
UNION ALL SELECT 'LlmUsageRecords', COUNT(*) FROM LlmUsageRecords
UNION ALL SELECT 'OutboundWebhookSubscriptions', COUNT(*) FROM OutboundWebhookSubscriptions
UNION ALL SELECT 'OutboundWebhookDeliveries', COUNT(*) FROM OutboundWebhookDeliveries
UNION ALL SELECT 'AgentProfiles', COUNT(*) FROM AgentProfiles
UNION ALL SELECT 'AgentRuns', COUNT(*) FROM AgentRuns
UNION ALL SELECT 'KnowledgeDocuments', COUNT(*) FROM KnowledgeDocuments
UNION ALL SELECT 'ExternalLogins', COUNT(*) FROM ExternalLogins
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The row-count verification SQL is missing several tables that are included in the migration steps. Missing tables: CardCommentMentions, CommandRunLogs, NotificationPreferences, AgentRunEvents, and KnowledgeChunks. These should be added to the checklist to ensure a complete integrity check.

Comment on lines +191 to +215
**DateTimeOffset handling**: SQLite stores `DateTimeOffset` as ISO 8601 text strings. PostgreSQL stores them as `timestamptz`. The CSV import should parse ISO 8601 strings correctly. If timezone information is missing from SQLite values, PostgreSQL will assume UTC.

## Step 4: Verify Data Integrity

Run row-count verification against PostgreSQL and compare with the pre-migration counts from the checklist:

```bash
psql "$PGCONN" <<'SQL'
SELECT 'Users' AS tbl, COUNT(*) FROM "Users"
UNION ALL SELECT 'Boards', COUNT(*) FROM "Boards"
UNION ALL SELECT 'Columns', COUNT(*) FROM "Columns"
UNION ALL SELECT 'Cards', COUNT(*) FROM "Cards"
UNION ALL SELECT 'Labels', COUNT(*) FROM "Labels"
UNION ALL SELECT 'CardLabels', COUNT(*) FROM "CardLabels"
UNION ALL SELECT 'CardComments', COUNT(*) FROM "CardComments"
UNION ALL SELECT 'BoardAccesses', COUNT(*) FROM "BoardAccesses"
UNION ALL SELECT 'AuditLogs', COUNT(*) FROM "AuditLogs"
UNION ALL SELECT 'AutomationProposals', COUNT(*) FROM "AutomationProposals"
UNION ALL SELECT 'AutomationProposalOperations', COUNT(*) FROM "AutomationProposalOperations"
UNION ALL SELECT 'ArchiveItems', COUNT(*) FROM "ArchiveItems"
UNION ALL SELECT 'ChatSessions', COUNT(*) FROM "ChatSessions"
UNION ALL SELECT 'ChatMessages', COUNT(*) FROM "ChatMessages"
UNION ALL SELECT 'CommandRuns', COUNT(*) FROM "CommandRuns"
UNION ALL SELECT 'Notifications', COUNT(*) FROM "Notifications"
UNION ALL SELECT 'UserPreferences', COUNT(*) FROM "UserPreferences"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The post-migration row-count verification SQL is missing the same set of tables as the pre-migration checklist (CardCommentMentions, CommandRunLogs, NotificationPreferences, AgentRunEvents, and KnowledgeChunks). Including these is necessary to verify that all data was successfully transferred to PostgreSQL.

@Chris0Jeky
Copy link
Copy Markdown
Owner Author

Adversarial Review -- PR #801

CRITICAL Issues

C1. Runbook Step 1 will FAIL on PostgreSQL: SQLite-specific raw SQL in migrations

  • File: Referenced backend/src/Taskdeck.Infrastructure/Migrations/20260328234951_AddKnowledgeDocumentsAndFts.cs, lines 85-91
  • The runbook says to run dotnet ef database update against PostgreSQL (Step 1). However, migration AddKnowledgeDocumentsAndFts contains raw SQLite-specific SQL:
    migrationBuilder.Sql(@"CREATE VIRTUAL TABLE IF NOT EXISTS KnowledgeDocumentsFts USING fts5(...)");
    migrationBuilder.Sql(@"CREATE TRIGGER IF NOT EXISTS KnowledgeDocuments_ai ...");
    These statements will throw a PostgresException because PostgreSQL has no CREATE VIRTUAL TABLE or FTS5 syntax. The Down() method also has SQLite-specific SQL for the FTS virtual table.
  • Impact: The runbook's Step 1 is non-functional. An operator following it will hit an error and have no guidance on how to proceed.
  • Fix: The runbook must document that this migration requires provider-conditional wrapping (if (migrationBuilder.ActiveProvider == "Microsoft.EntityFrameworkCore.Sqlite")), OR document a manual workaround (e.g., skip FTS creation on PostgreSQL and note it as a TODO for the PostgreSQL FTS implementation). This is the single biggest blocker for anyone actually attempting the migration.

C2. Runbook references non-existent ApiKeys table

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, lines 55, 127, 169, and Step 4 verification
  • The ApiKeys table does NOT exist in TaskdeckDbContext. There is no DbSet<ApiKey> anywhere in the codebase. Searching for ApiKey in the Infrastructure project returns zero results.
  • Impact: The pre-migration count query (line 55) will fail with no such table: ApiKeys. The export command (line 127) will fail. The import will try to import a non-existent CSV. The verification query will fail against PostgreSQL.
  • Fix: Remove ApiKeys from all four runbook sections (pre-migration checklist, export, import, verification).

C3. Pre-migration checklist (Step 3) missing 5 tables that exist in the schema

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, lines 29-56
  • The pre-migration row-count query omits these DbContext tables:
    1. AgentRunEvents (FK child of AgentRuns)
    2. CardCommentMentions (FK child of CardComments)
    3. CommandRunLogs (FK child of CommandRuns)
    4. KnowledgeChunks (FK child of KnowledgeDocuments)
    5. NotificationPreferences
  • These ARE present in the export/import steps, so the pre-migration baseline would be incomplete. The operator would have no reference counts for these 5 tables when verifying Step 4.
  • Impact: An operator cannot verify data integrity for 5 tables because they have no baseline counts.
  • Fix: Add all 5 missing tables to the pre-migration count query.

C4. Step 4 verification missing 3 tables that are exported and imported

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, Step 4 row-count verification (lines 198-227)
  • Missing from the PostgreSQL verification query:
    1. AgentRunEvents
    2. CardCommentMentions
    3. KnowledgeChunks
  • These are exported in Step 2 and imported in Step 3, but the operator cannot verify they imported correctly.
  • Impact: Data loss in these 3 child tables would go undetected.
  • Fix: Add these tables to the Step 4 verification query.

HIGH Issues

H1. Test harness only tests SQLite -- provides zero cross-provider validation

  • File: backend/tests/Taskdeck.Api.Tests/DatabaseProviderCompatibilityTests.cs
  • The class docstring says "When the environment variable TASKDECK_TEST_POSTGRES_CONNECTION is set, a parallel PostgreSQL run can be configured via a provider-switching factory." However, this is entirely aspirational:
    • The env var TASKDECK_TEST_POSTGRES_CONNECTION is never read in any code (only in comments/docs).
    • TestWebApplicationFactory always uses UseSqlite() with no conditional PostgreSQL path.
    • There is no [Trait], [ConditionalFact], or skip logic for PostgreSQL vs SQLite.
  • Impact: All 20 tests run exclusively against SQLite. The "compatibility harness" validates nothing about PostgreSQL. The PR title and ADR claim dual-provider testing, but the actual deliverable is SQLite-only tests with PostgreSQL documentation.
  • Fix: At minimum, be explicit in the PR description and docs that the PostgreSQL path is NOT implemented -- it is a future TODO. Better: add a skeleton conditional in the test factory that reads the env var and switches to UseNpgsql(), even if CI doesn't have a PostgreSQL instance yet. This would make the env var claim truthful.

H2. ConcurrentInserts_DoNotLoseData test name is misleading -- tests sequential inserts

  • File: backend/tests/Taskdeck.Api.Tests/DatabaseProviderCompatibilityTests.cs, lines 751-786
  • The test creates 20 cards in a loop on a single DbContext instance with a single SaveChangesAsync() call. This is a batch insert, not concurrent writes. The comment (line 764) says "SQLite doesn't support true concurrent writes" but then uses this as justification for a sequential test.
  • A real concurrency test would use multiple DbContext instances, multiple threads/tasks, and detect DbUpdateConcurrencyException or SQLiteException (database is locked). The current test would pass even if the database had severe concurrency bugs.
  • Fix: Rename to BatchInsert_DoesNotLoseData to accurately describe what it tests. Add a TODO for actual multi-context concurrency testing against PostgreSQL.

H3. Runbook FTS migration guidance is insufficient

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, lines 319-322
  • The "Full-Text Search Migration Note" section says the migration "requires a PostgreSQL-specific implementation of that interface" but provides zero concrete guidance:
    • No SQL for creating tsvector columns or GIN indexes
    • No mention that the FTS5 virtual table (KnowledgeDocumentsFts) should NOT be exported/imported
    • No mention that the SQLite trigger (KnowledgeDocuments_ai) won't exist in PostgreSQL
    • No mention of pg_trgm as a simpler alternative for basic text search
  • Fix: Either expand the FTS section with concrete PostgreSQL setup commands, or explicitly mark it as out-of-scope with a reference to a future issue. At minimum, note that KnowledgeDocumentsFts is a SQLite FTS5 virtual table that does NOT migrate and should be excluded from export.

MEDIUM Issues

M1. ADR-0023 says "row-count and checksum verification" but runbook has no checksum mechanism

  • File: docs/decisions/ADR-0023-sqlite-to-postgresql-migration-strategy.md, Decision section, point 4
  • The ADR text says "row-count and foreign-key integrity verification" in the runbook, but the ADR decision section says "row-count and checksum verification." No checksum mechanism is implemented.
  • Fix: Change ADR wording to "row-count and foreign-key integrity verification" to match the runbook.

M2. Runbook does not disable FK constraints or triggers during import

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, Step 3
  • The import uses \COPY with FKs enabled. If any table has a self-referential FK or the dependency order has a subtle error, the import will fail mid-stream with an FK violation. PostgreSQL best practice for bulk data loads is to disable FK triggers temporarily.
  • Fix: Add SET session_replication_role = replica; before the import loop (disables FK checking) and SET session_replication_role = DEFAULT; after, followed by a manual FK integrity check.

M3. String Contains test makes misleading provider equivalence claim

  • File: backend/tests/Taskdeck.Api.Tests/DatabaseProviderCompatibilityTests.cs, lines 344-376
  • The test comment says "SQLite LIKE is case-insensitive for ASCII, so both match" (line 373). But on PostgreSQL, Contains("login") would translate to LIKE '%login%' which is case-SENSITIVE. So "Add LOGIN feature" would NOT match on PostgreSQL. The test uses BeGreaterThanOrEqualTo(1) which masks this provider difference.
  • This is exactly the kind of divergence the compatibility harness should flag, but it's hidden behind a weak assertion.
  • Fix: The assertion should document the expected difference clearly. Use [Fact] with a comment saying "SQLite: 2 matches (case-insensitive LIKE), PostgreSQL: 1 match (case-sensitive LIKE)". The weak assertion BeGreaterThanOrEqualTo(1) should be tightened to Be(2) for the SQLite path, with a TODO for the PostgreSQL path.

LOW Issues

L1. Runbook connection string in environment variable exposes password in process list

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, lines 74, 266
  • export ConnectionStrings__DefaultConnection="Host=...;Password=<password>" makes the password visible in /proc/<pid>/environ and ps -e output on Linux.
  • Fix: Recommend using PGPASSFILE (~/.pgpass) for psql commands and an appsettings file with restricted permissions for the .NET application, rather than environment variables containing passwords.

L2. Runbook PGCONN variable uses key=value format with password inline

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md, line 172
  • PGCONN="host=<host> dbname=taskdeck user=taskdeck_app password=<password>" makes the password visible in shell history and ps output.
  • Fix: Use PGPASSFILE or suggest psql environment variables (PGHOST, PGDATABASE, PGUSER) with .pgpass for the password.

L3. No mention of sequence resetting for auto-increment columns

  • File: docs/platform/SQLITE_TO_POSTGRES_MIGRATION_RUNBOOK.md
  • If any tables use SERIAL/IDENTITY columns (auto-incrementing integers), PostgreSQL sequences need to be reset after CSV import to the max value + 1. The current schema uses GUIDs for PKs (no sequences), but the runbook should note this for future-proofing.
  • Fix: Add a note: "The current schema uses GUID primary keys, so no sequence resets are needed. If future tables use integer PKs, add SELECT setval(...) after import."

Summary

Severity Count Key themes
CRITICAL 4 Migration will fail on PostgreSQL (FTS5 SQL), phantom table, missing tables in verification
HIGH 3 Tests are SQLite-only despite claiming dual-provider, misleading concurrency test, incomplete FTS guidance
MEDIUM 3 ADR/runbook text mismatch, no FK deferral during import, weak Contains assertion
LOW 3 Password exposure, connection format, sequence note

The ADR is well-written and the alternatives analysis is thorough. The test harness is well-structured and covers useful patterns. The core problem is that the runbook has data integrity gaps (missing/phantom tables, FTS migration blocker) and the test harness's "dual-provider" claim is aspirational rather than implemented.

Chris0Jeky and others added 3 commits April 9, 2026 04:05
Runbook fixes:
- Remove phantom ApiKeys table from all 4 sections (does not exist in schema)
- Add 5 missing tables to pre-migration count query (AgentRunEvents,
  CardCommentMentions, CommandRunLogs, KnowledgeChunks, NotificationPreferences)
- Add 3 missing tables to Step 4 verification (AgentRunEvents,
  CardCommentMentions, KnowledgeChunks)
- Add FTS5 migration blocker warning to Step 1 with provider-conditional
  guard guidance
- Expand FTS migration note with KnowledgeDocumentsFts exclusion guidance
- Add session_replication_role FK deferral during bulk import

Test harness fixes:
- Update class docstring to honestly state SQLite-only (PostgreSQL is TODO)
- Rename ConcurrentInserts_DoNotLoseData to BatchInsert_DoesNotLoseData
  with accurate docstring explaining it is not a concurrency test
- Fix Contains test: EF Core Contains() is case-sensitive on both providers;
  tighten assertion from BeGreaterThanOrEqualTo(1) to Be(1) with accurate
  provider behavior documentation
@Chris0Jeky Chris0Jeky merged commit d4fbe3b into main Apr 12, 2026
24 checks passed
@Chris0Jeky Chris0Jeky deleted the chore/sqlite-to-prod-db-migration-plan branch April 12, 2026 00:04
@github-project-automation github-project-automation bot moved this from Pending to Done in Taskdeck Execution Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

PLAT-01: SQLite-to-production DB migration strategy and compatibility harness

3 participants