fix: run the SSL-mode migration from the --mcp server process (#639)

[aesslinger]

Closes #639

Summary

The stale ssl_mode-spelling migration added in #614 only ran from the two GUI-process Tauri commands that load connections (get_connections / get_connections_with_groups). It was never invoked from the standalone tabularis --mcp server process, which reads the same connections.json and uses ssl_mode directly to establish real database connections. A connection saved with a stale MySQL-style ssl_mode value before #614's dropdown fix shipped stayed silently cleartext forever via MCP, even after the GUI-side fix landed, as long as it was never reopened in the GUI.

What changed

Four commits, each independently buildable/testable:

1. refactor: extract SSL-mode migration into a path-based shared module — moves the migration logic out of commands.rs into a new connection_migrations.rs module. The orchestration function is split into a path-based core (migrate_postgres_ssl_mode_spelling_at_path, no AppHandle dependency) plus a thin AppHandle wrapper left in commands.rs for the GUI's cache-invalidation need. Pure extraction, no behavior change — except the return type improves from Result<()> to Result<bool> so callers can tell "nothing needed migrating" apart from "migrated and saved."

2. fix: guard the SSL-mode migration against a concurrent-write race — this fix makes MCP a writer of connections.json for the first time (previously read-only there). Since MCP clients spawn tabularis --mcp as an independent subprocess that can run alongside the (single-instance) GUI app, and this codebase's connection persistence layer has no file locking, adds a content-based compare-and-swap: the file's raw content is checked immediately before writing, and the migration skips itself for this run (rather than overwriting) if it changed since the initial read. Safe because the migration is idempotent and self-terminating — the next process to load connections will retry.

3. fix: run the SSL-mode migration from the --mcp server process — wires the shared core into register_drivers_for_mcp(), appended after plugin loading completes (same ordering constraint the GUI path already has, since dialect resolution depends on the driver registry). Errors are swallowed the same way the existing GUI call site already does — migration failure must never block server startup.

4. refactor: avoid a redundant file read in the SSL-mode migration guard — found during local review: the guard read connections.json twice before comparing (once directly, once again inside persistence::load_connections_file). Not a correctness bug — traced every ordering and a concurrent edit landing in the gap still gets caught by the final comparison — but an unneeded extra syscall and a slightly wider window than necessary. Splits load_connections_file's parsing logic into a new persistence::parse_connections_file(&str) so the migration can parse the content it already read, without touching any of that function's ~20 other call sites.

Explicitly out of scope

• migrate_ssh_connections has this same gap (never called from MCP either) but is meaningfully more complex — a separate ssh_connections.json file plus OS keychain credential migration. Left as a follow-up decision, not fixed here.
• Real file locking across the whole persistence layer — the correct long-term fix for the concurrency risk, but would touch ~20 existing write call sites, not just this one. The compare-and-swap guard added here only protects this specific new write path.

Full design rationale, alternatives considered, and why each was rejected are documented on #639 before implementation began.

Verification

• cargo build --all-targets — 0 warnings
• cargo clippy --all-targets — 0 findings in touched files (129 pre-existing elsewhere, confirmed present on a clean main checkout)
• cargo fmt --check — clean on all touched files
• cargo test --lib — 1138/1138 (4 pre-existing environment-only skips)
• cargo test --test postgres_integration --include-ignored — 181/181
• Real-plugin parity (POSTGRES_PLUGIN_BIN against a live v1.0.0-beta.3 binary) — 83/83
• Manual end-to-end: built the actual tabularis --mcp binary and ran it against an isolated $HOME with the real installed plugin binary and a hand-written connections.json covering all 3 driver cases. Confirmed the exact expected outcome: the postgresql-driven connection's stale required rewrote to require (with the [Migration] Rewriting stale ssl_mode spelling on 1 postgres-dialect connection(s) log line), while mysql and builtin postgres connections were left untouched. Second run confirmed idempotent (no log line, no further changes).

[aesslinger]

Local verification notes

Ran a local review pass against .rules/rust.md and verified every claim in the PR description against the actual code (not just skimmed the diff):

Rules compliance — all satisfied: pure helpers extracted to a dedicated sibling module (connection_migrations.rs), tests in a sibling _tests.rs file loaded via #[cfg(test)] pub mod, all 3 pure functions have direct unit coverage, and the one type-signature change (Result<()> → Result<bool>) is behavior-preserving for both existing GUI call sites (traced through — cache invalidation happens under the exact same condition before and after).

Blast radius — full grep of every moved/new function name confirms nothing outside the touched files references them; no external callers missed.

One nuance found and fixed: migrate_postgres_ssl_mode_spelling_at_path was reading connections.json twice before comparing (once directly, once again inside load_connections_file). Traced every possible timing of a concurrent edit — not a correctness bug, since the final content_before vs content_now comparison still catches it — but an unnecessary extra read. Fixed in the 4th commit by splitting load_connections_file's parsing into a reusable parse_connections_file(&str), with zero signature change for its ~20 other call sites.

Manual end-to-end verification: built the real tabularis --mcp binary and ran it against an isolated $HOME with the actual installed PostgreSQL plugin binary and a hand-written connections.json covering all 3 driver cases (plugin with a stale value, MySQL with the same-but-correct value, builtin Postgres). Confirmed via the process's own log output and a file diff:

Connection	Before	After	Result
postgresql (plugin)	required	require	✅ rewritten
mysql	required	required	✅ correctly untouched
postgres (builtin)	required	required	✅ correctly untouched (by design)

Second run produced no [Migration] log line and no further changes — confirmed idempotent.

No blockers found. All test/build/lint claims in the PR description were independently reproduced, not taken on faith.

[kilo-code-bot]

SUGGESTION: The comment lists save-and-report-true as one of the file-level behaviors these tests exercise, but no test in this block actually drives migrate_postgres_ssl_mode_spelling_at_path through its write path (the Ok(true) return after save_connections_file). Only the two no-op cases are present. The dialect-resolution logic is well covered by the migrate_connection_ssl_mode_in_place tests above, but the path-based function's own happy path (rewrite to disk + return Ok(true)) and its concurrent-change skip branch (connection_migrations.rs:162-170) have no integration coverage here — connections_file_changed_concurrently is unit-tested in isolation, but its wiring into the path-based function isn't. Either add a test that seeds a stale ssl_mode and asserts the file is rewritten + Ok(true) (a driver-id other than "postgres" would require a registered driver, which is why these tests avoid it — but a fixture/registered driver would close the gap), or correct the comment so it doesn't overstate coverage.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review of commit 75ddc0e (the only change since the prior review at 6a556c7). The previously-flagged test comment in connection_migrations_tests.rs — which overstated coverage by listing save-and-report-true as an exercised path — has been rewritten to accurately document that only the two no-op paths are covered, and to explicitly call out the uncovered Ok(true) rewrite-and-save path and concurrent-change skip branch, with rationale (requires a non-"postgres" driver id resolving to Postgres dialect, needing a live plugin or a 62-method DatabaseDriver mock). No new issues introduced by the change.

Files Reviewed (1 file)

• src-tauri/src/connection_migrations_tests.rs — 0 issues (comment-only change; prior SUGGESTION resolved)

Previous Review Summary (commit 6a556c7)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 6a556c7)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	0
WARNING	0
SUGGESTION	1

Issue Details (click to expand)

SUGGESTION

File	Line	Issue
src-tauri/src/connection_migrations_tests.rs	191	Test comment overstates coverage — claims a save-and-report-true file-level test exists, but only the two no-op cases are present. The path-based function's write path (Ok(true)) and the concurrent-change skip branch are untested at the integration level.

Files Reviewed (6 files)

• src-tauri/src/commands.rs - 0 issues (pure extraction; moved code out to shared module, GUI wrapper preserves cache-invalidation behavior and matches existing .ok() error-swallowing pattern at the call sites)
• src-tauri/src/connection_migrations.rs - 0 issues (path-based core, content-based CAS guard is correctly proportionate and well-documented; parse_connections_file reuse avoids the redundant read; imports resolve)
• src-tauri/src/connection_migrations_tests.rs - 1 issue
• src-tauri/src/lib.rs - 0 issues (module declarations)
• src-tauri/src/mcp/mod.rs - 0 issues (wiring uses the same paths::resolve_connections_path(&paths::get_app_config_dir()) resolution the rest of MCP uses for connections; runs after driver registration as required; .ok() matches GUI call sites)
• src-tauri/src/persistence.rs - 0 issues (behavior-preserving split of parse_connections_file out of load_connections_file; ~20 other call sites unaffected)

Fix these issues in Kilo Cloud

---

_{Reviewed by glm-5.2 · Input: 46.1K · Output: 7.5K · Cached: 156K}