From 4bcb3794c5e7a5e16f02dd7031813bb3c0bed0bf Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Thu, 6 Aug 2026 20:14:04 +0200 Subject: [PATCH] fix(plugin-kit): the SQLite reader never opened anything on Linux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while running the lutris plugin's own release gate against a live host on .21: `parity --compare` reported `1 missing` — the plugin produced NO entry for the one game Lutris had, while the host's built-in scanner produced it fine. `scan` agreed: "present: 0 games". `detect` still said "present", because detect only stats the file. `openReadOnly` builds a `file:?immutable=1` URI — the right idea, since `immutable=1` is what makes this a pure read that cannot lock a running launcher's database or spawn WAL sidecars next to it. But it opened that name with `{ readonly: true }`, and the options object does NOT enable SQLite's URI filename parsing. Without SQLITE_OPEN_URI the name is taken literally, no such file exists, and the open throws: SQLiteError: unable to open database file Every path here then degrades that to silence by design: `openReadOnly` returns `undefined` for "this launcher isn't installed", `withReadOnlyDb` passes the `undefined` through, and callers write `withReadOnlyDb(...) ?? []`. So a total, permanent failure to read ANY database was indistinguishable from an empty library. The lutris plugin reported 0 games on every Linux box, always. It is platform-split, which is why it survived review and local runs: macOS links Apple's system SQLite, which is built with URI filenames enabled, so the same call succeeds there. Linux uses Bun's bundled SQLite, which is not. MEASURED both ways on bun 1.3.14. Fixed by passing the flags explicitly — SQLITE_OPEN_READONLY | SQLITE_OPEN_URI. (`{ readonly: true, uri: true }` is not a supported option shape; also measured.) And the second half, which is why nothing caught it: `parity --compare` sets `process.exitCode = 1` on a mismatch and then returns NORMALLY — a red parity is a finished comparison, not a crashed command. `runPluginCli` then assigned `process.exitCode = 0` unconditionally after the effect resolved, overwriting it. So the one verb both plugin READMEs document as the release gate — "exits non-zero on any difference", "do not publish a version whose parity run is red" — always exited 0, and any scripted use of it passed. Now `??= 0`, so a code a command set deliberately survives. Tests: the sqlite helper had NO coverage at all, which is the whole reason a total failure shipped looking like an empty library. Added five cases against a REAL database file — reads rows back, handles a path needing URI escaping, withReadOnlyDb round-trips, an absent file is `undefined` not a throw, and a bad query still degrades to []. Verified they actually catch it: against the shipped code on Linux, 4 of the 5 fail; with the fix, 21/21 in that file and 61/61 across the suite, typecheck and build clean. End to end on .21 with the fix: lutris `scan` goes 0 -> 1 games and `parity --compare` reports "parity OK — 1 entries identical". A deliberately doctored baseline now exits 1 instead of 0. 0.3.0 -> 0.3.1. --- plugin-kit/package.json | 2 +- plugin-kit/src/cli.ts | 8 ++- plugin-kit/src/library/parsers/sqlite.ts | 19 +++++- plugin-kit/test/library-parsers.test.ts | 79 ++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 4 deletions(-) diff --git a/plugin-kit/package.json b/plugin-kit/package.json index c53efb8e..c4cf6fad 100644 --- a/plugin-kit/package.json +++ b/plugin-kit/package.json @@ -1,6 +1,6 @@ { "name": "@punktfunk/plugin-kit", - "version": "0.3.0", + "version": "0.3.1", "description": "Effect-based framework for punktfunk plugins: lifecycle runtime, config/state, sync engine, UI serving, CLI scaffold, and browser helpers.", "type": "module", "license": "MIT OR Apache-2.0", diff --git a/plugin-kit/src/cli.ts b/plugin-kit/src/cli.ts index fcad0385..2535a5e6 100644 --- a/plugin-kit/src/cli.ts +++ b/plugin-kit/src/cli.ts @@ -73,7 +73,13 @@ export const runPluginCli = async (opts: { const rt = ManagedRuntime.make(Layer.provideMerge(opts.def.layer, base)); try { await rt.runPromise(Effect.scoped(command.run(rest))); - process.exitCode = 0; + // Do NOT clobber a non-zero code the command set deliberately. `parity --compare` reports a + // mismatch by setting `process.exitCode = 1` and then RETURNING normally — a red parity is a + // finished comparison, not a crashed command. Assigning 0 here unconditionally overwrote it, + // so the one verb documented as a release gate ("exits non-zero on any difference", "do not + // publish a version whose parity run is red") always exited 0, and any scripted use of it + // passed. MEASURED against a live host on 2026-08-06: `parity FAILED — 1 missing`, exit 0. + process.exitCode ??= 0; } catch (e) { const hint = e instanceof HostRequestError diff --git a/plugin-kit/src/library/parsers/sqlite.ts b/plugin-kit/src/library/parsers/sqlite.ts index 6831796b..df08298b 100644 --- a/plugin-kit/src/library/parsers/sqlite.ts +++ b/plugin-kit/src/library/parsers/sqlite.ts @@ -4,9 +4,22 @@ // opened it read-write could take a write lock, create `-wal`/`-shm` sidecars next to it, or (worst // case) be blamed for a corrupted library. `immutable=1` promises the file will not change while // open, which makes Bun skip locking entirely — the strictest possible "look, don't touch". -import { Database } from "bun:sqlite"; +import { constants, Database } from "bun:sqlite"; import { isFile } from "./fs.js"; +/** + * READONLY | URI, passed as raw open flags. + * + * The `{ readonly: true }` options object does NOT enable SQLite's URI filename parsing, so a + * `file:…?immutable=1` name is taken literally, no such file exists, and the open throws + * `SQLiteError: unable to open database file`. Every caller here degrades an open failure to + * "launcher not installed", so that turned into a silent, total "0 games" on every box — see the + * regression test in test/library-parsers.test.ts. SQLITE_OPEN_URI is what makes the query string + * mean anything. (`{ readonly: true, uri: true }` is not a thing — measured on bun 1.3.14.) + */ +const OPEN_READONLY_URI = + constants.SQLITE_OPEN_READONLY | constants.SQLITE_OPEN_URI; + export interface ReadOnlyDb { /** Run a query and return its rows. Returns `[]` rather than throwing on a bad query. */ readonly query: >( @@ -29,7 +42,9 @@ export const openReadOnly = (file: string): ReadOnlyDb | undefined => { // `readonly` alone still takes locks and can spawn WAL sidecars; `immutable=1` is what makes // this a pure read. It is safe here precisely because a scan is a point-in-time snapshot — // if the launcher writes mid-scan we simply pick it up on the next sync. - db = new Database(`file:${encodeURI(file)}?immutable=1`, { readonly: true }); + // The flags (not `{ readonly: true }`) are load-bearing: without SQLITE_OPEN_URI the name + // below is not parsed as a URI and the open always fails. See OPEN_READONLY_URI. + db = new Database(`file:${encodeURI(file)}?immutable=1`, OPEN_READONLY_URI); } catch { return undefined; } diff --git a/plugin-kit/test/library-parsers.test.ts b/plugin-kit/test/library-parsers.test.ts index 8c038cd3..ae290c7d 100644 --- a/plugin-kit/test/library-parsers.test.ts +++ b/plugin-kit/test/library-parsers.test.ts @@ -6,6 +6,7 @@ // per-plugin parity harness (design M5) then checks the whole pipeline against a live host, but // these catch a drift long before that. import { describe, expect, test } from "bun:test"; +import { Database } from "bun:sqlite"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; @@ -17,6 +18,8 @@ import { fileUrl, gridFilenames, isSteamTool, + withReadOnlyDb, + openReadOnly, parseAppManifest, parseRegQuery, parseShortcuts, @@ -287,3 +290,79 @@ describe("reg.exe output", () => { ]); }); }); + +// The read-only SQLite helper, against a REAL database file. +// +// This exists because its absence shipped a total failure. `openReadOnly` built a +// `file:…?immutable=1` URI but opened it with `{ readonly: true }`, which does not enable SQLite's +// URI filename parsing — so the name was taken literally, the open threw, and `openReadOnly` +// returned `undefined`. Every caller reads that as "this launcher isn't installed", and +// `withReadOnlyDb(...) ?? []` turns it into an empty library. The lutris plugin therefore reported +// "0 games" on every box, forever, while `detect` still said "present" (it only stats the file) — +// and the only thing that caught it was a hand-run parity gate against a live host. +// +// So: assert the helper can actually READ, not merely that it returns something. +describe("openReadOnly", () => { + const withDb = (use: (file: string) => T): T => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pf-kit-sqlite-")); + const file = path.join(dir, "pga.db"); + const seed = new Database(file); + seed.run("CREATE TABLE games (id INTEGER PRIMARY KEY, name TEXT, installed INT)"); + seed.run("INSERT INTO games (id, name, installed) VALUES (1, 'Ubisoft Connect', 1)"); + seed.close(); + try { + return use(file); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }; + + test("opens a real database and returns its rows", () => { + withDb((file) => { + const db = openReadOnly(file); + expect(db).toBeDefined(); + expect(db?.query("SELECT id, name FROM games WHERE installed = 1")).toEqual([ + { id: 1, name: "Ubisoft Connect" }, + ]); + db?.close(); + }); + }); + + // A path with a space is the realistic URI-encoding case (Flatpak roots, "Program Files"). + test("opens a path that needs URI escaping", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pf kit sqlite ")); + const file = path.join(dir, "pga.db"); + const seed = new Database(file); + seed.run("CREATE TABLE games (id INTEGER PRIMARY KEY)"); + seed.run("INSERT INTO games (id) VALUES (7)"); + seed.close(); + try { + expect(openReadOnly(file)?.query("SELECT id FROM games")).toEqual([{ id: 7 }]); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + test("withReadOnlyDb reads, then closes", () => { + withDb((file) => { + expect(withReadOnlyDb(file, (h) => h.query("SELECT name FROM games"))).toEqual([ + { name: "Ubisoft Connect" }, + ]); + }); + }); + + // The "not installed" contract — an absent file is `undefined`, never a throw. + test("absent file is undefined, not an error", () => { + expect(openReadOnly(path.join(os.tmpdir(), "pf-kit-nope", "pga.db"))).toBeUndefined(); + expect(withReadOnlyDb(path.join(os.tmpdir(), "pf-kit-nope", "pga.db"), () => 1)).toBeUndefined(); + }); + + // Schema drift degrades to no rows rather than taking the plugin down. + test("a bad query returns [] rather than throwing", () => { + withDb((file) => { + const db = openReadOnly(file); + expect(db?.query("SELECT missing_column FROM games")).toEqual([]); + db?.close(); + }); + }); +});