refactor: extract CatalogDB interface for WASM portability

[retran]

Why

mdl/catalog/ and mdl/linter/ depend directly on *sql.DB and modernc.org/sqlite. To compile mxcli as a WASM module (for embedding in Studio Pro's Maia client), the catalog layer needs to delegate SQL operations to the host process instead. This PR introduces an interface boundary so a future JsCatalogDB implementation can swap in without touching any consumer code.

Stacked on #274 — merge #274 first, then rebase this PR.

Changes

• catalogdb.go — CatalogDB interface (Query, QueryRow, Exec, Begin, Close) + CatalogTx interface (Prepare, Exec, QueryRow, Commit, Rollback)
• catalogdb_sqlite.go — SqliteCatalogDB wrapping *sql.DB (//go:build !js), with RawDB() escape hatch and WrapSqlDB() test helper
• catalog.go — db *sql.DB → CatalogDB, DB() → CatalogDB(), New() delegates to NewSqliteCatalogDB(), NewFromDB() constructor added, SaveToFile uses type assertion
• builder.go — tx *sql.Tx → CatalogTx
• linter/context.go — db *sql.DB → catalog.CatalogDB, DB() → CatalogDB()
• rules/missing_translations.go — ctx.DB() → ctx.CatalogDB()
• Test files — wrap *sql.DB via catalog.WrapSqlDB(), contract tests for both interfaces, compile-time interface satisfaction checks

Design decisions

• Interface returns *sql.Rows / *sql.Row to avoid rewriting 18 linter scan sites
• CatalogTx.Prepare() returns *sql.Stmt — keeps Builder's bulk insert pattern unchanged
• SaveToFile stays SQLite-specific via type assertion (uses VACUUM INTO)
• JsCatalogDB stub deferred to follow-up PR — needs WASM bridge design

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• Type assertion in SaveToFile: The SaveToFile method in catalog.go uses a type assertion to *SqliteCatalogDB to access SQLite-specific functionality (VACUUM INTO). While this is acknowledged in the PR description as intentional for now (with a WASM implementation deferred), it creates a temporary tight coupling to SQLite. Consider adding a comment explaining why this is acceptable temporarily, or define an interface method for file persistence that SQLite implements.

What Looks Good

• Clean abstraction: The CatalogDB and CatalogTx interfaces properly abstract the database/sql operations used throughout the catalog and linter packages.
• Consistent refactor: All usages of *sql.DB and *sql.Tx in the catalog, builder, and linter packages have been consistently updated to use the new interfaces.
• Good test coverage: Added comprehensive contract tests for both interfaces, plus compile-time interface satisfaction checks.
• Proper escape hatch: The RawDB() method on SqliteCatalogDB provides access to SQLite-specific functionality when needed (like VACUUM INTO in SaveToFile), maintaining flexibility.
• Test-friendly: Added WrapSqlDB() helper to make it easy for tests to wrap their own *sql.DB instances.
• Clear separation: Native CLI continues to use modernc.org/sqlite via SqliteCatalogDB, while leaving room for a future WASM-backed implementation.
• Minimal behavior change: The refactor maintains identical functionality while improving architectural flexibility for future WASM portability.

Recommendation

The PR is ready to be merged. It successfully extracts the database interfaces needed for WASM portability while maintaining all existing functionality through careful, consistent refactoring. The minor concern about the type assertion in SaveToFile is explicitly acknowledged as temporary and justified for SQLite-specific operations, with a clear path forward for the WASM implementation.

Approve the PR.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[Copilot]

Pull request overview

This PR refactors catalog and linter database access behind new CatalogDB / CatalogTx interfaces (to enable non-database/sql-backed implementations later), and continues the executor cleanup by removing ExecContext’s Executor back-pointer in favor of explicit context fields + sync-back.

Changes:

• Introduce CatalogDB / CatalogTx interfaces and a native SQLite implementation (SqliteCatalogDB) plus test helpers/contract tests.
• Update catalog + linter code (and tests) to consume CatalogDB rather than *sql.DB.
• Update executor handlers/tests to rely on ExecContext state + syncBack rather than ctx.executor back-references.

Reviewed changes

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

Show a summary per file

File	Description
mdl/catalog/catalogdb.go	Adds CatalogDB / CatalogTx interfaces.
mdl/catalog/catalogdb_sqlite.go	Adds SqliteCatalogDB implementation (!js) + RawDB() + WrapSqlDB().
mdl/catalog/catalog.go	Switches catalog to CatalogDB, adds NewFromDB, updates SaveToFile to use SQLite-specific escape hatch.
mdl/catalog/builder.go	Switches builder transaction type from *sql.Tx to CatalogTx.
mdl/catalog/catalogdb_test.go	Adds contract/interface satisfaction tests.
mdl/linter/context.go	Switches linter context DB field/accessors to catalog.CatalogDB.
mdl/linter/rules/*	Updates rule + tests to use CatalogDB and WrapSqlDB.
mdl/executor/*	Removes back-pointer usage, adds sync-back and explicit ExecContext callbacks/fields.

Comments suppressed due to low confidence (1)

mdl/executor/cmd_catalog.go:466

• In REFRESH CATALOG ... BACKGROUND, the goroutine builds the catalog using a shallow-copied bgCtx. That avoids races, but it also means bgCtx.Catalog is never propagated back to the live session/Executor, while the handler already cleared ctx.Catalog. Net effect: after a background build finishes (and even prints "Catalog ready"), subsequent commands still see ctx.Catalog == nil until the user manually reloads from cache. If the intent is to keep background refresh usable in-session, consider returning the built catalog via a channel/callback to the main goroutine (or avoiding clearing the existing catalog until the replacement is ready) and then atomically swapping ctx.Catalog/syncing state back.

	// Close existing catalog if any
	if ctx.Catalog != nil {
		ctx.Catalog.Close()
		ctx.Catalog = nil
	}

	// Handle background mode — clone ctx so the goroutine doesn't race
	// with the main dispatch loop (which may syncBack and mutate fields).
	// NOTE: bgCtx.Output still shares the underlying writer with the main
	// goroutine. This is a pre-existing limitation — the original code also
	// wrote to ctx.Output from the goroutine. A synchronized writer would
	// fix this but is out of scope for the executor cleanup.
	if stmt.Background {
		bgCtx := *ctx   // shallow copy — isolates scalar fields
		bgCtx.Cache = nil // detach shared cache so preWarmCache writes stay local
		go func() {
			if err := buildCatalog(&bgCtx, stmt.Full, stmt.Source); err != nil {
				fmt.Fprintf(bgCtx.Output, "Background catalog build failed: %v\n", err)
			}
		}()
		fmt.Fprintln(ctx.Output, "Catalog build started in background...")
		return nil
	}

---

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

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

mdl/executor/cmd_catalog.go:466

• REFRESH CATALOG ... BACKGROUND now builds the catalog on a shallow-copied bgCtx, but the resulting bgCtx.Catalog is never propagated back to the live session/executor state. Because the handler also clears ctx.Catalog before spawning the goroutine, the session can end up with no in-memory catalog even after the background build completes (only the cache file is written). Consider adding an explicit sync-back mechanism for background work (e.g., a SetCatalog/SyncBack callback on ExecContext, or sending the built catalog back to the main goroutine via a channel) so the built catalog becomes available without requiring an extra reload command.

	// Handle background mode — clone ctx so the goroutine doesn't race
	// with the main dispatch loop (which may syncBack and mutate fields).
	// NOTE: bgCtx.Output still shares the underlying writer with the main
	// goroutine. This is a pre-existing limitation — the original code also
	// wrote to ctx.Output from the goroutine. A synchronized writer would
	// fix this but is out of scope for the executor cleanup.
	if stmt.Background {
		bgCtx := *ctx   // shallow copy — isolates scalar fields
		bgCtx.Cache = nil // detach shared cache so preWarmCache writes stay local
		go func() {
			if err := buildCatalog(&bgCtx, stmt.Full, stmt.Source); err != nil {
				fmt.Fprintf(bgCtx.Output, "Background catalog build failed: %v\n", err)
			}
		}()
		fmt.Fprintln(ctx.Output, "Catalog build started in background...")
		return nil
	}

---

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

[ako]

Code Review — PR #277 (retrospective, already merged)

Overview: Extracts a CatalogDB/CatalogTx interface pair so the catalog can be swapped from *sql.DB (SQLite) to a WASM-compatible backend without touching callers. Good incremental approach.

---

What works well

• //go:build !js on catalogdb_sqlite.go cleanly isolates the SQLite dependency
• Compile-time interface check var _ CatalogDB = (*SqliteCatalogDB)(nil) is good practice
• WrapSqlDB() gives tests a real CatalogDB without mocking
• NewFromDB(CatalogDB) constructor is clean
• SaveToFile type-assertion is documented and safe for now

---

Design gap: CatalogTx is not WASM-portable

CatalogTx.Prepare() returns *sql.Stmt, a concrete database/sql type:

type CatalogTx interface {
    Prepare(query string) (*sql.Stmt, error)
    ...
}

A future JsCatalogTx cannot satisfy this interface without wrapping a real *sql.Stmt. That defeats the stated WASM portability goal — the interface will need revision before a JS backend can implement it. Consider either:

• Removing Prepare from the interface (inline the query instead), or
• Introducing a CatalogStmt interface and returning that instead

Same issue applies to Query returning *sql.Rows and QueryRow returning *sql.Row — those are also concrete types. For now this is fine (incremental step), but worth tracking before the JS backend is written.

---

Minor points

• WrapSqlDB() is exported but only used in tests (_test.go files). Consider making it wrapSqlDB (unexported) or moving it to a _test.go file to avoid polluting the package surface.
• Error message wrapping was removed from NewSqliteCatalogDB — the caller now gets a bare SQLite error with no context. The old "failed to open in-memory database: %w" wrapper was useful.

---

Overall: Solid first step. The leaky abstraction is documented and acceptable for now, but CatalogTx.Prepare() returning *sql.Stmt should be addressed before implementing a real WASM backend.