From b3c7552d7f1e9ad46b4b723b3a3ef734e02dd845 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 15:32:33 +0000 Subject: [PATCH] fix(kb): stop --dry-run creating the index tables, and report them in db stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The knowledge base's three tables are built lazily against the `getDb()` connection instead of through `createStorage`, so neither dry-run guard reached them: no `ReadOnlyTabularStorage` wrapper in the path, and this module's one `isDryRun()` protected the `kb_index` row write several lines below the three `CREATE TABLE`s. `sec --dry-run ask` created all three on a database that had none — DDL from a command whose whole promise is to change nothing. A dry run against an index that already exists is the ordinary case and still reads it. One that would have to build the index now refuses and names `sec index`, rather than quietly building it. The same bypass had a second consequence the issue documents: `db stats` derives its rows from the storage registry, these three are not in it, and there is no repository token to count them through — so an operator had no way to see whether an index existed or how large it was. `getKbTableStats` counts them off the connection directly and the report appends them: `n/a` before the index is built, the same signal a registered-but-uncreated table reports, and omitted entirely where the index cannot exist (a non-SQLite backend, or before the SQLite location is bound — one unreadable appendix must not cost the operator every row count above it). Registry adoption, which would retire the exception outright, is left open on the issue: `defineStorage` cannot yet express a vector storage at a fixed width. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LWp6Z6wvAPDaDCjAFcTSj6 --- CHANGELOG.md | 10 +++++ src/cli/queries/DbStatus.ts | 5 +++ src/kb/kbTableStats.test.ts | 80 +++++++++++++++++++++++++++++++++ src/kb/kbTableStats.ts | 55 +++++++++++++++++++++++ src/kb/secKnowledgeBase.test.ts | 69 +++++++++++++++++++++++++++- src/kb/secKnowledgeBase.ts | 44 ++++++++++++++++-- 6 files changed, 258 insertions(+), 5 deletions(-) create mode 100644 src/kb/kbTableStats.test.ts create mode 100644 src/kb/kbTableStats.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 20a9e183..73432e52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,16 @@ they care about is running `embarc-data`, which is unaffected. - `typecheck` covers src, scripts and tests in one pass; `typecheck-tests` and `tsconfig.test.json` are gone. CI runs it before the build. - `sec fetch golden-fixtures` is now `bun run check-fixtures`. +- **`--dry-run` no longer creates the knowledge-base tables.** Those three are + built lazily against the `getDb()` connection rather than through + `createStorage`, so no `ReadOnlyTabularStorage` wrapper stood between a dry + run and three `CREATE TABLE`s. A dry run against an existing index still reads + it; one that would have to build the index now says so and names `sec index`. +- **`db stats` reports the knowledge-base tables.** They have no repository + token to count through, so the report derived from the storage registry could + not see them and an operator had no way to tell whether an index existed. They + are counted directly and appended, `n/a` before the index is built, and + omitted entirely on a backend where the index cannot exist. ## 0.1.5 diff --git a/src/cli/queries/DbStatus.ts b/src/cli/queries/DbStatus.ts index 652a675b..0ab48b24 100644 --- a/src/cli/queries/DbStatus.ts +++ b/src/cli/queries/DbStatus.ts @@ -1,6 +1,7 @@ import type { ServiceToken } from "workglow"; import { globalServiceRegistry } from "workglow"; import { SEC_STORAGE_REGISTRY } from "../../config/storageRegistry"; +import { getKbTableStats } from "../../kb/kbTableStats"; import { CIK_NAME_REPOSITORY_TOKEN } from "../../storage/entity/CikNameSchema"; import { ENTITY_REPOSITORY_TOKEN } from "../../storage/entity/EntitySchema"; import { COMPANY_FACTS_REPOSITORY_TOKEN } from "../../storage/facts/CompanyFactsSchema"; @@ -243,5 +244,9 @@ export async function getDbStats( `counted ${table} (${current}/${tables.length})` ); } + // Appended rather than merged into TABLE_TOKENS: the knowledge base's tables + // have no repository token to count through, and leaving them out is what + // left an operator no way to see whether an index existed at all. + results.push(...(await getKbTableStats())); return results; } diff --git a/src/kb/kbTableStats.test.ts b/src/kb/kbTableStats.test.ts new file mode 100644 index 00000000..2ab8bf24 --- /dev/null +++ b/src/kb/kbTableStats.test.ts @@ -0,0 +1,80 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { globalServiceRegistry } from "workglow"; +import { withSqliteDb } from "../config/testing/withSqliteDb"; +import { resetDependencyInjectionsForTesting } from "../config/TestingDI"; +import { SEC_DB_TYPE } from "../config/tokens"; +import { getKbTableStats } from "./kbTableStats"; +import { SEC_KB_TABLE_NAMES } from "./secKbTables"; +import { getSecKnowledgeBase, resetSecKnowledgeBaseForTesting } from "./secKnowledgeBase"; + +/** + * `db stats` loops the storage registry, and the knowledge base's three tables + * are deliberately not in it — they are built lazily against the `getDb()` + * connection. The consequence was that an operator had no way to see whether an + * index existed or how large it was. + */ +describe("getKbTableStats", () => { + withSqliteDb("kb_stats", []); + + beforeEach(async () => { + await resetSecKnowledgeBaseForTesting(); + delete process.env.SEC_EMBEDDING_MODEL; + }); + + afterEach(async () => { + await resetSecKnowledgeBaseForTesting(); + delete process.env.SEC_EMBEDDING_MODEL; + }); + + it("reports every knowledge-base table as n/a before the index is built", async () => { + // `null` is the same "registered, not created" signal the registry tables + // use, so the report reads the same whether or not an index exists. + expect(await getKbTableStats()).toEqual( + SEC_KB_TABLE_NAMES.map((table) => ({ table, rows: null, estimated: false })) + ); + }); + + it("counts the tables once the index exists", async () => { + await getSecKnowledgeBase(); + + const stats = await getKbTableStats(); + + expect(stats.map((s) => s.table)).toEqual([...SEC_KB_TABLE_NAMES]); + // kb_index carries the one row recording which model built the index; the + // document and chunk tables are created empty. + expect(stats.find((s) => s.table === "kb_index")?.rows).toBe(1); + expect(stats.find((s) => s.table === "kb_document")?.rows).toBe(0); + expect(stats.find((s) => s.table === "kb_chunk")?.rows).toBe(0); + expect(stats.every((s) => s.estimated === false)).toBe(true); + }); +}); + +describe("getKbTableStats on a non-SQLite backend", () => { + beforeEach(() => resetDependencyInjectionsForTesting()); + afterEach(() => resetDependencyInjectionsForTesting()); + + it("reports nothing rather than n/a", async () => { + // The index is SQLite-only by design — `getSecKnowledgeBase` refuses + // Postgres by name. Three permanent `n/a` rows would read as a setup gap an + // operator could close, and there is nothing to close. + globalServiceRegistry.registerInstance(SEC_DB_TYPE, "postgres"); + expect(await getKbTableStats()).toEqual([]); + }); +}); + +describe("getKbTableStats with no SQLite location bound", () => { + beforeEach(() => resetDependencyInjectionsForTesting()); + afterEach(() => resetDependencyInjectionsForTesting()); + + it("reports nothing rather than opening a database from an unset folder", async () => { + // `db stats` degrades one unreadable table to n/a rather than losing the + // whole report; an appendix that cannot be read at all is the same promise. + expect(await getKbTableStats()).toEqual([]); + }); +}); diff --git a/src/kb/kbTableStats.ts b/src/kb/kbTableStats.ts new file mode 100644 index 00000000..3b3fd41f --- /dev/null +++ b/src/kb/kbTableStats.ts @@ -0,0 +1,55 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { globalServiceRegistry } from "workglow"; +import type { TableStat } from "../cli/queries/DbStatus"; +import { SEC_DB_FOLDER, SEC_DB_NAME, SEC_DB_TYPE } from "../config/tokens"; +import { getDb } from "../util/db"; +import { SEC_KB_TABLE_NAMES } from "./secKbTables"; + +/** + * Row counts for the knowledge base's three tables. + * + * `db stats` derives its rows from the storage registry, and these three are + * not in it: they are a vector store at a fixed width plus its two companions, + * built lazily against the connection `getDb()` owns. Counting them through a + * repository token is therefore not available — there is no token — so they are + * counted here and appended to the report. + * + * Returns nothing on a non-SQLite backend. The index is SQLite-only by design + * and `getSecKnowledgeBase` refuses Postgres by name, so three permanent `n/a` + * rows would read as a setup gap rather than as an unavailable feature. Also + * nothing when the SQLite location is unbound: `getDb()` would open a file + * from an unset folder token, and one unreportable appendix must not cost the + * operator every row count above it. + * + * A table the index has not been built for counts `null`, the same signal a + * registered table the database has not created reports. + */ +export async function getKbTableStats(): Promise { + const backend = globalServiceRegistry.has(SEC_DB_TYPE) + ? globalServiceRegistry.get(SEC_DB_TYPE) + : "sqlite"; + if (backend !== "sqlite") return []; + if (!globalServiceRegistry.has(SEC_DB_FOLDER) || !globalServiceRegistry.has(SEC_DB_NAME)) { + return []; + } + + const db = getDb(); + const present = new Set( + ( + db.prepare("SELECT name FROM sqlite_master WHERE type = 'table'").all() as { name: string }[] + ).map((row) => row.name) + ); + + return SEC_KB_TABLE_NAMES.map((table) => { + if (!present.has(table)) return { table, rows: null, estimated: false }; + // Safe to interpolate: every name comes from SEC_KB_TABLE_NAMES, which is + // three compile-time constants. SQLite cannot bind a relation name. + const counted = db.prepare(`SELECT COUNT(*) AS n FROM "${table}"`).all() as { n: number }[]; + return { table, rows: Number(counted[0]?.n ?? 0), estimated: false }; + }); +} diff --git a/src/kb/secKnowledgeBase.test.ts b/src/kb/secKnowledgeBase.test.ts index 746d3201..00d77ddc 100644 --- a/src/kb/secKnowledgeBase.test.ts +++ b/src/kb/secKnowledgeBase.test.ts @@ -9,7 +9,7 @@ import { globalServiceRegistry } from "workglow"; import { resetAllDatabases } from "../config/resetAllDatabases"; import { resetDependencyInjectionsForTesting } from "../config/TestingDI"; import { withSqliteDb } from "../config/testing/withSqliteDb"; -import { SEC_DB_TYPE } from "../config/tokens"; +import { SEC_DB_TYPE, SEC_DRY_RUN } from "../config/tokens"; import { SEC_EMBEDDING_DIMENSIONS } from "../config/models"; import { getDb } from "../util/db"; import { KB_INDEX_TABLE, SEC_KB_TABLE_NAMES } from "./secKbTables"; @@ -203,3 +203,70 @@ describe("the SEC knowledge base's chunk search", () => { expect(hits.map((hit) => hit.chunk_id)).toEqual(["east", "north"]); }); }); + +/** + * `--dry-run` promises to show what would happen without changing anything. + * These three tables are built lazily against the `getDb()` connection rather + * than through `createStorage`, so neither guard that protects every other + * table reaches them: no `ReadOnlyTabularStorage` wrapper, and the `isDryRun()` + * in this module guarded only the `kb_index` row write, several lines below the + * three `CREATE TABLE`s. + */ +describe("the knowledge base under --dry-run", () => { + withSqliteDb("kb_dry", []); + + function kbTablesOnDisk(): string[] { + const rows = getDb() + .prepare("SELECT name FROM sqlite_master WHERE type = 'table' AND name LIKE 'kb_%'") + .all() as { name: string }[]; + return rows.map((row) => row.name).sort(); + } + + beforeEach(async () => { + await resetSecKnowledgeBaseForTesting(); + delete process.env.SEC_EMBEDDING_MODEL; + }); + + afterEach(async () => { + await resetSecKnowledgeBaseForTesting(); + globalServiceRegistry.registerInstance(SEC_DRY_RUN, false); + delete process.env.SEC_EMBEDDING_MODEL; + }); + + it("creates no table when the index does not exist yet", async () => { + globalServiceRegistry.registerInstance(SEC_DRY_RUN, true); + + await expect(getSecKnowledgeBase()).rejects.toThrow(/dry run/i); + + expect(kbTablesOnDisk()).toEqual([]); + }); + + it("names the command that would build the index", async () => { + globalServiceRegistry.registerInstance(SEC_DRY_RUN, true); + await expect(getSecKnowledgeBase()).rejects.toThrow(/sec index/); + }); + + it("opens an index that already exists, and still writes nothing", async () => { + // A dry run against a real database is the normal case: the tables are + // there, and the run must be allowed to read them. + globalServiceRegistry.registerInstance(SEC_DRY_RUN, false); + await getSecKnowledgeBase(); + await resetSecKnowledgeBaseForTesting(); + expect(kbTablesOnDisk()).toEqual(SEC_KB_TABLE_NAMES.toSorted()); + + getDb().exec(`DELETE FROM ${KB_INDEX_TABLE}`); + globalServiceRegistry.registerInstance(SEC_DRY_RUN, true); + + await expect(getSecKnowledgeBase()).resolves.toBeDefined(); + const rows = getDb().prepare(`SELECT COUNT(*) AS n FROM ${KB_INDEX_TABLE}`).all() as { + n: number; + }[]; + expect(rows[0]?.n).toBe(0); + }); + + it("still creates the tables when this is not a dry run", async () => { + globalServiceRegistry.registerInstance(SEC_DRY_RUN, false); + await getSecKnowledgeBase(); + expect(kbTablesOnDisk()).toEqual(SEC_KB_TABLE_NAMES.toSorted()); + }); +}); diff --git a/src/kb/secKnowledgeBase.ts b/src/kb/secKnowledgeBase.ts index 8ce9aac7..51cc1c84 100644 --- a/src/kb/secKnowledgeBase.ts +++ b/src/kb/secKnowledgeBase.ts @@ -24,7 +24,12 @@ import { SecCliConfigurationError } from "../config/EnvToDI"; import { secEmbeddingModel, SEC_EMBEDDING_DIMENSIONS } from "../config/models"; import { SEC_DB_TYPE } from "../config/tokens"; import { getDb } from "../util/db"; -import { KB_CHUNK_TABLE, KB_DOCUMENT_TABLE, KB_INDEX_TABLE } from "./secKbTables"; +import { + KB_CHUNK_TABLE, + KB_DOCUMENT_TABLE, + KB_INDEX_TABLE, + SEC_KB_TABLE_NAMES, +} from "./secKbTables"; /** The one knowledge base, under the id `sec ask` resolves it by. */ export const SEC_KB_ID = "sec"; @@ -98,6 +103,21 @@ async function requireMatchingEmbeddingModel( ); } +/** + * Which of the knowledge base's tables the database does not have. + * + * Read off `sqlite_master` rather than by probing each storage: a `SELECT` + * against a missing table throws, and distinguishing "no such table" from a + * real failure by its message is the sort of guess this can simply avoid. + */ +function missingKbTables(db: { prepare(sql: string): { all(): unknown[] } }): string[] { + const rows = db.prepare("SELECT name FROM sqlite_master WHERE type = 'table'").all() as { + name: string; + }[]; + const present = new Set(rows.map((row) => row.name)); + return SEC_KB_TABLE_NAMES.filter((table) => !present.has(table)); +} + /** * The knowledge base `sec index` fills and `sec ask` reads. * @@ -148,9 +168,25 @@ export async function getSecKnowledgeBase(): Promise { SEC_EMBEDDING_DIMENSIONS ); const index = new SqliteTabularStorage(db, KB_INDEX_TABLE, KbIndexSchema, KbIndexPrimaryKeyNames); - await documents.setupDatabase(); - await chunks.setupDatabase(); - await index.setupDatabase(); + // `setupDatabase()` is DDL, and these three are the only tables that reach it + // without passing through `createStorage` — so no `ReadOnlyTabularStorage` + // wrapper stands between a dry run and a `CREATE TABLE`. A dry run against an + // index that already exists is the ordinary case and reads it; one that would + // have to build the index says so instead of quietly building it. + if (isDryRun()) { + const missing = missingKbTables(db); + if (missing.length > 0) { + throw new SecCliConfigurationError( + `This is a dry run, and the knowledge base has no ${missing.join(", ")} ` + + `table yet. Creating one would be a change, so there is nothing to read: ` + + `run \`sec index\` without --dry-run to build the index first.` + ); + } + } else { + await documents.setupDatabase(); + await chunks.setupDatabase(); + await index.setupDatabase(); + } const model = secEmbeddingModel(); // Before the knowledge base is handed out, so a mismatch cannot be discovered