diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index e9c1f4552..94c70656c 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -302,6 +302,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/doctor.ts` — `gbrain doctor [--json] [--fast] [--fix] [--dry-run] [--index-audit]`: health checks. Checks include `jsonb_integrity` + `markdown_body_completeness` (reliability), `schema_version` (fails loudly when `version=0`, routes to `gbrain apply-migrations --yes`), `queue_health` (Postgres-only: stalled-forever active jobs started_at > 1h, waiting-depth-per-name > threshold default 10 via `GBRAIN_QUEUE_WAITING_THRESHOLD`, and dead-lettered subagent jobs with `last_error` matching the `prompt_too_long` classifier in last 24h), `sync_failures` (`[CODE=N, ...]` breakdown for unacked-warn + acked-ok; severity comes from the shared `decideSyncFailureSeverity` in `src/core/sync-failure-ledger.ts` so the LOCAL and REMOTE/thin-client doctor surfaces can never drift — a stuck bookmark escalates to FAIL once an OPEN failure has blocked past the staleness window or ≥10 files block, while already `auto_skipped` rows stay a visible WARN), `rls_event_trigger` (healthy `evtenabled` set is `('O','A')` only; fix hint `gbrain apply-migrations --force-retry 35`), `graph_coverage` (short-circuits to ok when `SELECT COUNT(*) FROM pages WHERE type IN ('entity','person','company','organization')` returns 0; WARN hint is `gbrain extract all`), `embedding_column_registry` (probes each declared column via Postgres `format_type(atttypid, atttypmod)` to catch dim mismatch with a paste-ready `gbrain config set embedding_columns '{...}'` hint, probes HNSW index presence via `pg_indexes`, computes default-column population via `COUNT(*) FILTER (WHERE IS NOT NULL) / COUNT(*)` warning below 90% except empty brains where chunk_count=0 short-circuits to ok; PGLite parity via `executeRaw`), and `skill_brain_first` (walks SKILL.md via `autoDetectSkillsDirReadOnly`, calls `analyzeSkillBrainFirst()` from `src/core/skill-brain-first.ts` per file with structured `Check.issues[]`; warn states `missing_brain_first`/`brain_first_typo`, ok states `compliant_callout`/`compliant_phase`/`compliant_position`/`exempt_frontmatter`/`no_external`; snapshot+diff audit at `~/.gbrain/audit/skill-brain-first-YYYY-Www.jsonl`). `--fix` delegates inlined cross-cutting rules to `> **Convention:** see [path](path).` callouts via `src/core/dry-fix.ts` (and MISSING_RULE_PATTERNS for the brain-first callout); `--fix --dry-run` previews. `--index-audit` (Postgres-only, informational, no auto-drop) reports zero-scan indexes from `pg_stat_user_indexes`. Every DB check runs under a progress phase; `markdown_body_completeness` runs under a 1s heartbeat. `runDoctor` uses `autoDetectSkillsDirReadOnly` (from `src/core/repo-root.ts`; install-path fallback so `cd ~ && gbrain doctor` finds bundled skills); `--fix` carries a D6 install-path safety gate that refuses auto-repair when `detected.source === 'install_path'` (would rewrite the bundled tree). The Lane D supervisor check at `doctor.ts:1011-1043` consumes `summarizeCrashes(events)` from `src/core/minions/handlers/supervisor-audit.ts` (warn at `>=1` real crash; ok message has `clean_exits_24h=N`; warn message has `runtime=A oom=B unknown=C legacy=D` per-cause breakdown) so OOM/runtime/unknown crashes are distinguishable from clean code=0 worker drains; cross-surface parity with `gbrain jobs supervisor status` is pinned by source-grep wiring assertions requiring the breakdown substrings in BOTH `doctor.ts` and `jobs.ts`. `checkSyncFreshness` (exported, in `runDoctor` local + `doctorReportRemote` thin-client) is a staleness probe: warns at 24h, fails at 72h or never-synced; future-`last_sync_at` warns ("clock skew") instead of falling through ok; env overrides `GBRAIN_SYNC_FRESHNESS_WARN_HOURS`/`GBRAIN_SYNC_FRESHNESS_FAIL_HOURS` (invalid fall back with once-per-process stderr warn via `_resolveSyncFreshnessHours`); failure messages embed `source.id` so the printed `gbrain sync --source ` matches. A source holding a LIVE, non-expired per-source sync lock (`inspectLock(engine, syncLockId(source.id))` from `src/core/db-lock.ts`) is reported as actively syncing (the message names the holder pid + host) and counted in `synced_recently_count`, NOT flagged stale — the live lock is the only honest in-progress signal (checkpoint banking can't distinguish in-progress from wedged: a blocked sync banks its files but writes no anchor). A blocked/failed sync's process has exited (no lock row) and a wedged holder stops refreshing (TTL lapses), so either falls through to the stale path and is never masked; the dynamic `db-lock` import is swallowed to a no-op on a stub engine or pre-lock-table brain, so this can only ADD an in-progress verdict, never suppress a real stale one. The in-progress note is appended to whatever verdict the buckets produce and is empty when nothing is syncing, so steady-state messages stay byte-for-byte unchanged. It has a `localOnly`-gated git short-circuit (`runDoctor` passes `localOnly: true`; `doctorReportRemote` runs in the HTTP MCP server `src/commands/serve-http.ts` and keeps default `false` so that path never walks DB-supplied `local_path` via subprocess — trust boundary). The local predicate mirrors sync's "do work?" gate (HEAD == `last_commit` AND working tree clean via `requireCleanWorkingTree: 'ignore-untracked'` so a quiet repo with only untracked dirs is `unchanged` not SEVERE, AND `chunker_version === CURRENT`); the inline SELECT carries `last_commit + chunker_version + newest_content_at`. The REMOTE path computes lag via `lagFromContentMs(newest_content_at, lastSync, now)` from the stored column, NO git subprocess; LOCAL fall-through and the `< 0` clock-skew check stay on raw wall-clock. Three-bucket count math populates `Check.details = {unchanged_count, synced_recently_count, stale_count}` with the invariant `sum === sources.length`. `checkCycleFreshness` is DELIBERATELY NOT git-short-circuited or content-relativized (`last_commit == HEAD` can't answer "did the full cycle complete?"; a sync can succeed while later cycle phases fail; different axis `last_full_cycle_at`). Pinned by `test/doctor.test.ts` (incl. IRON-RULE regression banning stale verb names, the sync_freshness boundary matrix, the D4 regression guard verifying git probes are NEVER called when `localOnly` is unset/false, the three-bucket invariant, and the untracked-folders / remote-never-shells-out trust-boundary cases). - `src/core/migrate.ts` — schema-migration runner. Owns the `MIGRATIONS` array (source of truth for schema DDL). `Migration` interface carries `sqlFor?: { postgres?, pglite? }` (engine-specific SQL overrides `sql`) and `transaction?: boolean` (false for `CREATE INDEX CONCURRENTLY`, which Postgres refuses in a transaction; ignored on PGLite). Key migrations: v14 (handler branches on `engine.kind` for CONCURRENTLY-on-Postgres with invalid-remnant pre-drop via `pg_index.indisvalid`, plain `CREATE INDEX` on PGLite); v15 (`minion_jobs.max_stalled` default 1→5 + backfill non-terminal rows); v24 `rls_backfill_missing_tables` (`sqlFor: { pglite: '' }` no-op — PGLite has no RLS engine, targets subagent tables absent from pglite-schema.ts); v30 `dream_verdicts (file_path TEXT, content_hash TEXT, worth_processing BOOL, reasons JSONB, judged_at TIMESTAMPTZ, PK(file_path, content_hash))` (RLS-enabled under BYPASSRLS; synthesize reads/writes to avoid re-judging); v35 auto-RLS event trigger `auto_rls_on_create_table` fires on `ddl_command_end` for `WHEN TAG IN ('CREATE TABLE','CREATE TABLE AS','SELECT INTO')` running `ALTER TABLE … ENABLE ROW LEVEL SECURITY` on new `public.*` tables (no FORCE) + one-time backfill on every existing `public.*` base table whose comment doesn't match `^GBRAIN:RLS_EXEMPT\s+reason=\S.{3,}` (per-table failure aborts the offending CREATE TABLE; no EXCEPTION wrap; PGLite no-op via `sqlFor.pglite: ''`; breaking change: intentionally-RLS-off public tables need the GBRAIN:RLS_EXEMPT comment before upgrade); v40 `pages_emotional_weight` (`pages.emotional_weight REAL NOT NULL DEFAULT 0.0`, column-only metadata-only); v46 `mcp_request_log_params_jsonb_normalize` (`UPDATE ... SET params = params::text::jsonb WHERE jsonb_typeof(params) = 'string'`, idempotent); v60-v65 six-migration chain wiring source-scoping into `oauth_clients` — v60 (`oauth_clients_source_id_fk`: `source_id TEXT` NULL→`'default'` backfill + FK to `sources(id) ON DELETE SET NULL`), v61 (`federated_read TEXT[] NOT NULL DEFAULT '{}'`), v62 (explicit-CASE backfill so `source_id IS NULL` → `'{}'`), v63 (fail-loud check every row's source_id is in its federated_read array), v64 (FK flipped to `ON DELETE RESTRICT`), v65 (GIN index for array-containment); v68 `eval_candidates_embedding_column` (`eval_candidates.embedding_column TEXT NULL` per-row provenance for `gbrain eval replay` to reproduce the same retrieval space; NULL-tolerant); v108 `pages_embedding_signature` (`pages.embedding_signature TEXT NULL` = `:` stamped via `setPageEmbeddingSignature`; GRANDFATHER — stale predicate is `embedding_signature IS NOT NULL AND embedding_signature <> $current` so NULL is NEVER stale and upgrade never re-embeds the whole corpus; no index; metadata-only); v109 `sources_newest_content_at` (`sources.newest_content_at TIMESTAMPTZ` durable newest-COMMIT HEAD committer time written by `writeSyncAnchor`, read by the REMOTE staleness path instead of shelling to git; mirror in pglite-schema.ts + schema.sql + bootstrap probe); v110 `page_aliases` (`(id, source_id, alias_norm, slug, ...)` with `UNIQUE (source_id, alias_norm, slug)` + lookup indexes on `(source_id, alias_norm)` and `(source_id, slug)`; `alias_norm` is `normalizeAlias()` output so WRITE/READ key on the same form; also in `src/core/pglite-schema.ts`); v111 `search_telemetry_rank1_columns` (`ADD COLUMN IF NOT EXISTS` on both engines: `sum_rank1_score`, `count_rank1`, three buckets `rank1_lt_solid`/`rank1_solid`/`rank1_high` on `search_telemetry` — aggregate not per-query rows so rank-1 median drift is bounded-growth; ALTERs right after v57 which created the table); v114 `links_link_source_check_kebab_regex` (#1941, opens `link_source` from the closed allowlist to a kebab-case format gate `^[a-z][a-z0-9]*(-[a-z0-9]+)*$` + `char_length<=64`; Postgres branch uses `NOT VALID` + `VALIDATE CONSTRAINT` with `transaction:false`, PGLite plain DROP+ADD; existing built-ins all satisfy the regex so VALIDATE never fails on existing data); v116 `code_edges_source_backfill_and_callee_index` (#2073, idempotent: backfills NULL `code_edges_symbol`/`code_edges_chunk` `source_id` from each edge's `from_chunk` page — NULL never matched a scoped `AND source_id = …` filter so scoped `code-callers`/`code-callees` returned 0 rows on multi-source brains — plus plain `CREATE INDEX` on `from_symbol_qualified` for both edge tables, which had no index and seq-scanned per BFS node). The dedup-index self-heal (`timeline_dedup_index`, see `timeline-dedup-repair.ts`) is NOT version-gated: `runMigrations` invokes `repairTimelineDedupIndex` on every pass (including the no-pending early-return path) because a merge-renumbered migration can leave the version counter past the index change while the index stays the old shape. - `src/core/timeline-dedup-repair.ts` (#2038) — schema-drift self-heal for `idx_timeline_dedup`. The migration that widened the dedup index from `(page_id, date, summary)` to `(page_id, date, summary, source)` was renumbered during a master merge, so a brain that ran the old variant has its version counter stamped past the change while the index keeps the 3-column shape — and every `addTimelineEntry` batch then fails its 4-column `ON CONFLICT`, silently breaking timeline writes brain-wide. The version counter can't detect this, so the repair is keyed off the actual index SHAPE: `checkTimelineDedupIndex(engine)` returns `{tablePresent, indexPresent, columns, needsRepair}` (read-only; powers the `timeline_dedup_index` doctor check) and `repairTimelineDedupIndex(engine)` dedupes-then-rebuilds the index. `runMigrations` invokes the repair on every pass (including the no-pending early-return path); idempotent no-op when the index is already 4-column. `gbrain apply-migrations --force-schema` triggers it on demand. Pinned by `test/timeline-dedup-repair.test.ts`. +- `src/core/pages-unique-repair.ts` (#550) — schema-drift self-heal for the `pages(source_id, slug)` unique index, the arbiter both engines' `putPage` `ON CONFLICT (source_id, slug)` infers on. Migration v23 (Postgres) / v21 (PGLite) originally guarded the `ADD CONSTRAINT pages_source_slug_key` on `pg_constraint.conname` — a NAME match — so a brain stamped past v23 whose constraint never materialized (or was later dropped) is write-dead: every put_page throws "no unique or exclusion constraint matching the ON CONFLICT specification" while reads stay green, and neither `initSchema()` nor version-gated migrations can bring it back. The probe (`PAGES_SOURCE_SLUG_UNIQUE_PROBE_SQL`) matches BY COLUMNS via `pg_index`: any non-partial, non-expression unique index whose key columns are exactly `{source_id, slug}` (either order, constraint-backed or bare index) satisfies; partial indexes do NOT (inference rejects them). `checkPagesUniqueIndex(engine)` powers the `pages_unique_index` doctor check (OK/skip when the `pages` table or `source_id` column doesn't exist yet — a fresh/pre-v21 brain is not broken; FAIL prints the literal repair SQL `PAGES_UNIQUE_REPAIR_SQL`). `repairPagesUniqueIndex(engine)` applies the shared shape-guarded DDL (`PAGES_SOURCE_SLUG_UNIQUE_DDL`, also interpolated into the v21/v23 migration guards): no-op when any satisfying index exists under any name; falls back to constraint name `pages_source_slug_uniq` when `pages_source_slug_key` is taken by a wrong-shaped relation; never auto-dedupes pages (duplicate rows make ADD CONSTRAINT fail with the duplicated key named — deleting pages is the operator's call). `runMigrations` invokes the repair on every pass (same always-run slot as the #2038 timeline heal). Pinned by `test/pages-unique-repair.test.ts`. - `src/core/progress.ts` — Shared bulk-action progress reporter. Writes to stderr. Modes: `auto` (TTY `\r`-rewriting; non-TTY plain lines), `human`, `json` (JSONL), `quiet`. Rate-gated by `minIntervalMs` and `minItems`. `startHeartbeat(reporter, note)` for single long queries. `child()` composes phase paths. Singleton SIGINT/SIGTERM coordinator emits `abort` events for every live phase. EPIPE defense on both sync throws and stream `'error'` events. Zero dependencies. `emitHumanLine` is prefix-aware — inside a `withSourcePrefix(id, ...)` scope from `src/core/console-prefix.ts` it prepends `[id] ` (and TTY-rewrite mode `\r\x1b[2K` carries the prefix inside the clear-to-EOL escape); `emitJson` is intentionally NOT prefixed so NDJSON consumers don't choke on a `[id] {...}` shape. - `src/core/console-prefix.ts` — `AsyncLocalStorage`-backed per-source line-prefix helper. Exports `withSourcePrefix(id, fn)` (runs `fn` with `id` as active prefix; nested wraps replace then restore), `getSourcePrefix()` (read-only accessor; test seam), and `slog(...)` / `serr(...)` (prefix-aware `console.log`/`console.error`). Embedded-newline-safe: a multi-line string under prefix `[foo]` emits `[foo] line1\n[foo] line2`. Outside a wrap, `slog`/`serr` fall through to bare `console.log`/`console.error` so single-source callers see identical output (back-compat invariant). Use `src.id` (slug-validated by `sources add`) NOT `src.name` (free-form) to defeat log-injection through newline/control-character names. Coverage: `src/commands/sync.ts` performSync + callees, `src/commands/embed.ts` runEmbedCore + helpers, `src/core/progress.ts` emitHumanLine. - `src/core/cli-options.ts` — Global CLI flag parser. `parseGlobalFlags(argv)` returns `{cliOpts, rest}` with `--quiet` / `--progress-json` / `--progress-interval=` stripped. `getCliOptions()` / `setCliOptions()` expose a module-level singleton so commands reach resolved flags without parameter threading. `cliOptsToProgressOptions()` maps to reporter options. `childGlobalFlags()` returns the flag suffix to append to `execSync('gbrain ...')` calls in migration orchestrators. `OperationContext.cliOpts` extends shared-op dispatch for MCP callers. diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 1b514e18c..7056421c2 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -3,6 +3,7 @@ import { REPAIR_SOURCE_CONFIG_SQL } from '../core/source-config-sql.ts'; import { setCliExitVerdict } from '../core/cli-force-exit.ts'; import * as db from '../core/db.ts'; import { LATEST_VERSION, getIdleBlockers } from '../core/migrate.ts'; +import { checkPagesUniqueIndex, PAGES_UNIQUE_REPAIR_SQL } from '../core/pages-unique-repair.ts'; import { checkResolvable } from '../core/check-resolvable.ts'; import { autoFixDryViolations, type AutoFixReport, type FixOutcome } from '../core/dry-fix.ts'; import { autoDetectSkillsDirReadOnly } from '../core/repo-root.ts'; @@ -532,6 +533,59 @@ export async function childTableOrphansCheck(engine: BrainEngine): Promise { + try { + const status = await checkPagesUniqueIndex(engine); + if (!status.tablePresent) { + return { name: 'pages_unique_index', status: 'ok', message: 'no pages table yet' }; + } + if (!status.sourceIdPresent) { + return { + name: 'pages_unique_index', + status: 'ok', + message: 'pages.source_id not present yet (pre-v21 schema) — pending migrations own this', + }; + } + if (status.satisfied) { + return { + name: 'pages_unique_index', + status: 'ok', + message: `put_page upsert arbiter present (${status.indexName})`, + }; + } + return { + name: 'pages_unique_index', + status: 'fail', + message: + 'No non-partial UNIQUE index on pages(source_id, slug) — every put_page write fails with ' + + '"no unique or exclusion constraint matching the ON CONFLICT specification" (#550). ' + + `Fix (run against the brain database): ${PAGES_UNIQUE_REPAIR_SQL}`, + }; + } catch (e) { + return { + name: 'pages_unique_index', + status: 'warn', + message: `Could not check pages unique index: ${e instanceof Error ? e.message : String(e)}`, + }; + } +} + /** * Raw-source persistence guarantee (#1978, warn-only v1). * @@ -701,6 +755,11 @@ export async function doctorReportRemote(engine: BrainEngine): Promise = new Set([ 'ocr_health', 'orphan_ratio', 'oversized_pages', + 'pages_unique_index', 'quarantined_pages', 'raw_provenance', 'flagged_pages', diff --git a/src/core/migrate.ts b/src/core/migrate.ts index 085368246..eeb21965d 100644 --- a/src/core/migrate.ts +++ b/src/core/migrate.ts @@ -2,6 +2,7 @@ import type { BrainEngine } from './engine.ts'; import { slugifyPath } from './sync.ts'; import { getFtsLanguage } from './fts-language.ts'; import { hnswMaxDimsForType } from './vector-index.ts'; +import { PAGES_SOURCE_SLUG_UNIQUE_DDL } from './pages-unique-repair.ts'; /** * Schema migrations — run automatically on initSchema(). @@ -618,16 +619,12 @@ export const MIGRATIONS: Migration[] = [ // 0b. Swap pages.UNIQUE(slug) → UNIQUE(source_id, slug). // Deferred from v21 so PR #356 closes the integrity // window. PGLite already did this swap in its v21 path. + // #550: guarded by index SHAPE (columns), not constraint NAME — + // the old conname guard let a wrong-shaped or missing index pass + // as "done", leaving put_page's ON CONFLICT permanently broken. await tx.runMigration(23, ` ALTER TABLE pages DROP CONSTRAINT IF EXISTS pages_slug_key; - DO $$ BEGIN - IF NOT EXISTS ( - SELECT 1 FROM pg_constraint WHERE conname = 'pages_source_slug_key' - ) THEN - ALTER TABLE pages ADD CONSTRAINT pages_source_slug_key - UNIQUE (source_id, slug); - END IF; - END $$; + ${PAGES_SOURCE_SLUG_UNIQUE_DDL} `); // 1a. source_id with DEFAULT 'default' (idempotent) @@ -775,14 +772,7 @@ export const MIGRATIONS: Migration[] = [ CREATE INDEX IF NOT EXISTS idx_pages_source_id ON pages(source_id); ALTER TABLE pages DROP CONSTRAINT IF EXISTS pages_slug_key; - DO $$ BEGIN - IF NOT EXISTS ( - SELECT 1 FROM pg_constraint WHERE conname = 'pages_source_slug_key' - ) THEN - ALTER TABLE pages ADD CONSTRAINT pages_source_slug_key - UNIQUE (source_id, slug); - END IF; - END $$; + ${PAGES_SOURCE_SLUG_UNIQUE_DDL} `, }, }, @@ -6082,6 +6072,21 @@ export async function runMigrations(engine: BrainEngine): Promise<{ applied: num } } catch { /* best-effort; doctor reports the drift if this couldn't run */ } + // #550: same drift class for pages(source_id, slug). v23's old guard matched + // the constraint NAME, so a brain stamped >= v23 whose constraint never + // materialized (or was later dropped) is permanently write-dead — every + // put_page ON CONFLICT (source_id, slug) fails while reads look healthy. + // Shape-keyed, every-pass, idempotent; skips pre-v21 schemas (no source_id). + try { + const { repairPagesUniqueIndex } = await import('./pages-unique-repair.ts'); + const r = await repairPagesUniqueIndex(engine); + if (r.repaired) { + console.error( + `[migrate] healed missing pages(source_id, slug) unique index (#550) — put_page writes restored`, + ); + } + } catch { /* best-effort; the pages_unique_index doctor check reports it */ } + if (pending.length === 0) { return { applied: 0, current }; } diff --git a/src/core/pages-unique-repair.ts b/src/core/pages-unique-repair.ts new file mode 100644 index 000000000..ef9b99c73 --- /dev/null +++ b/src/core/pages-unique-repair.ts @@ -0,0 +1,135 @@ +/** + * #550 — pages(source_id, slug) unique-index drift detection + self-heal. + * + * Both engines' `putPage` upsert with `ON CONFLICT (source_id, slug)`. That + * inference only succeeds when a NON-PARTIAL unique index (constraint-backed + * or plain `CREATE UNIQUE INDEX`) exists whose key columns are exactly + * {source_id, slug}. When it's missing, EVERY put_page write fails with + * "there is no unique or exclusion constraint matching the ON CONFLICT + * specification" while reads keep working — the brain looks healthy and is + * write-dead. + * + * How the index goes missing: migration v23 (Postgres) / v21 (PGLite) guarded + * the `ADD CONSTRAINT pages_source_slug_key` on `pg_constraint.conname` — a + * NAME match, not a shape match. Any brain stamped >= v23 without that DDL + * actually executing (fresh-init blob stamped to head, externally-created + * `pages`, a later drop/rename) never gets it back: `runMigrations` early + * returns at head and `initSchema()` is additive-only. Same class as #2038 + * (idx_timeline_dedup), so the repair follows the same pattern: keyed off the + * actual index SHAPE, run on every migrate pass, idempotent. + */ + +import type { BrainEngine } from './engine.ts'; + +/** + * Selects the name of every index that satisfies `ON CONFLICT (source_id, + * slug)` inference on `pages`: + * - unique + * - non-partial (a partial unique index does NOT satisfy inference without + * a matching WHERE clause, which putPage doesn't emit) + * - plain-column (no expression keys) + * - key columns exactly {source_id, slug}, in any order + * + * Matched BY COLUMNS, never by name — a correctly-named but wrong-shaped + * index must not count. Shared verbatim by the doctor probe, the repair + * helper, and the v21/v23 migration guards (embedded in `EXISTS (...)`). + * `i.indkey` also lists INCLUDE columns, so with `indnkeyatts = 2` the + * aggregate has exactly 2 names only when there are no INCLUDE columns — + * conservative: an INCLUDE-bearing index is treated as not satisfying. + */ +export const PAGES_SOURCE_SLUG_UNIQUE_PROBE_SQL = ` + SELECT c.relname + FROM pg_index i + JOIN pg_class t ON t.oid = i.indrelid + JOIN pg_class c ON c.oid = i.indexrelid + WHERE t.relname = 'pages' + AND i.indisunique + AND i.indpred IS NULL + AND i.indexprs IS NULL + AND i.indnkeyatts = 2 + AND ( + SELECT array_agg(a.attname::text ORDER BY a.attname) + FROM pg_attribute a + WHERE a.attrelid = t.oid + AND a.attnum = ANY (i.indkey) + ) = ARRAY['slug', 'source_id'] +`; + +/** The literal operator repair — doctor prints this verbatim on FAIL. */ +export const PAGES_UNIQUE_REPAIR_SQL = + 'ALTER TABLE pages ADD CONSTRAINT pages_source_slug_key UNIQUE (source_id, slug);'; + +/** + * The guard DDL shared by migration v23 (Postgres) and v21 (PGLite) and by + * the every-pass self-heal. Column-shape-guarded, so: + * - a satisfying index under ANY name → no-op (no duplicate constraint) + * - the canonical name taken by a wrong-shaped relation → falls back to + * `pages_source_slug_uniq` instead of failing (ON CONFLICT inference is + * by shape; the name is not load-bearing) + * Does NOT dedupe pages first: if unconstrained duplicates snuck in, ADD + * CONSTRAINT fails naming the duplicated key — deleting pages automatically + * would be data loss, so that call stays with the operator. + */ +export const PAGES_SOURCE_SLUG_UNIQUE_DDL = ` + DO $$ BEGIN + IF NOT EXISTS (${PAGES_SOURCE_SLUG_UNIQUE_PROBE_SQL}) THEN + IF EXISTS (SELECT 1 FROM pg_class WHERE relname = 'pages_source_slug_key') THEN + ALTER TABLE pages ADD CONSTRAINT pages_source_slug_uniq UNIQUE (source_id, slug); + ELSE + ALTER TABLE pages ADD CONSTRAINT pages_source_slug_key UNIQUE (source_id, slug); + END IF; + END IF; + END $$; +`; + +export interface PagesUniqueStatus { + /** The pages table exists (nothing to check on a fresh brain). */ + tablePresent: boolean; + /** pages.source_id exists (pre-v21 schemas are the migration chain's job). */ + sourceIdPresent: boolean; + /** A satisfying unique index exists — put_page's ON CONFLICT works. */ + satisfied: boolean; + /** Name of the first satisfying index (null when unsatisfied). */ + indexName: string | null; +} + +export async function checkPagesUniqueIndex(engine: BrainEngine): Promise { + const tbl = await engine.executeRaw<{ reg: string | null }>( + `SELECT to_regclass('pages')::text AS reg`, + ); + if (!tbl[0]?.reg) { + return { tablePresent: false, sourceIdPresent: false, satisfied: false, indexName: null }; + } + const col = await engine.executeRaw<{ n: number }>( + `SELECT count(*)::int AS n FROM information_schema.columns + WHERE table_name = 'pages' AND column_name = 'source_id'`, + ); + if (Number(col[0]?.n ?? 0) === 0) { + return { tablePresent: true, sourceIdPresent: false, satisfied: false, indexName: null }; + } + const rows = await engine.executeRaw<{ relname: string }>(PAGES_SOURCE_SLUG_UNIQUE_PROBE_SQL); + return { + tablePresent: true, + sourceIdPresent: true, + satisfied: rows.length > 0, + indexName: rows[0]?.relname ?? null, + }; +} + +export interface PagesUniqueRepairResult { + repaired: boolean; + reason: 'already_correct' | 'no_table' | 'pre_source_id' | 'added'; +} + +/** + * Heal the missing unique index. Idempotent; a no-op on healthy brains and on + * pre-v21 schemas (no source_id yet — the ordinary migration chain owns those). + */ +export async function repairPagesUniqueIndex(engine: BrainEngine): Promise { + const status = await checkPagesUniqueIndex(engine); + if (!status.tablePresent) return { repaired: false, reason: 'no_table' }; + if (!status.sourceIdPresent) return { repaired: false, reason: 'pre_source_id' }; + if (status.satisfied) return { repaired: false, reason: 'already_correct' }; + await engine.executeRaw(PAGES_SOURCE_SLUG_UNIQUE_DDL); + return { repaired: true, reason: 'added' }; +} diff --git a/test/migrate.test.ts b/test/migrate.test.ts index f35daaff8..cb9ea774b 100644 --- a/test/migrate.test.ts +++ b/test/migrate.test.ts @@ -333,7 +333,10 @@ describe('migrate v23 — files_source_id_page_id_ledger', () => { expect(body).toContain('engine.transaction'); expect(body).toContain('files_page_slug_fkey'); expect(body).toContain('pages_slug_key'); - expect(body).toContain('pages_source_slug_key'); + // #550: the UNIQUE(source_id, slug) swap now routes through the shared + // column-shape-guarded DDL (src/core/pages-unique-repair.ts), so the + // handler body carries the interpolation marker instead of the literal. + expect(body).toContain('PAGES_SOURCE_SLUG_UNIQUE_DDL'); }); test('v23 backfills files.page_id scoped to default source (Codex fix)', () => { diff --git a/test/pages-unique-repair.test.ts b/test/pages-unique-repair.test.ts new file mode 100644 index 000000000..2fcaf07f6 --- /dev/null +++ b/test/pages-unique-repair.test.ts @@ -0,0 +1,184 @@ +/** + * #550 — pages(source_id, slug) unique-index drift: detection + self-heal. + * + * A brain stamped >= v23 whose `pages_source_slug_key` constraint never + * materialized (or was later dropped) fails EVERY put_page with "no unique or + * exclusion constraint matching the ON CONFLICT specification" while reads + * stay green. The old v21/v23 guards matched the constraint NAME, so + * re-running migrations could never repair it, and `initSchema()` was a + * no-op. These tests pin: the reproduction, the every-pass migrate self-heal + * (the behavioral fix — fails on master), shape-not-name detection (partial + * indexes rejected, differently-named satisfying indexes accepted), the + * name-collision fallback, and the doctor check's literal repair SQL. + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { runMigrations } from '../src/core/migrate.ts'; +import { + checkPagesUniqueIndex, + repairPagesUniqueIndex, + PAGES_UNIQUE_REPAIR_SQL, +} from '../src/core/pages-unique-repair.ts'; +import type { BrainEngine } from '../src/core/engine.ts'; + +// Dynamic so the module loads on a master checkout too — the discrimination +// proof for #550 runs this file against unmodified master, where doctor.ts +// has no pagesUniqueIndexCheck export and runMigrations has no heal. +async function doctorCheck(eng: BrainEngine) { + const doctor = await import('../src/commands/doctor.ts'); + if (typeof doctor.pagesUniqueIndexCheck !== 'function') { + throw new Error('doctor.ts does not export pagesUniqueIndexCheck (#550 check missing)'); + } + return doctor.pagesUniqueIndexCheck(eng); +} + +let engine: PGLiteEngine; +/** Schema-less engine (no initSchema) — the fresh-brain / no-pages-table case. */ +let bare: PGLiteEngine; +let n = 0; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + bare = new PGLiteEngine(); + await bare.connect({}); +}); + +afterAll(async () => { + await engine.disconnect(); + await bare.disconnect(); +}); + +/** One put_page upsert — the exact write path #550 kills. */ +function put() { + n++; + return engine.putPage( + `notes/issue-550-${n}`, + { type: 'note', title: `t${n}`, compiled_truth: `body ${n}` }, + { sourceId: 'default' }, + ); +} + +/** Wedge the brain: drop every arbiter the repair may have added. */ +async function dropArbiters() { + await engine.executeRaw(`ALTER TABLE pages DROP CONSTRAINT IF EXISTS pages_source_slug_key`); + await engine.executeRaw(`ALTER TABLE pages DROP CONSTRAINT IF EXISTS pages_source_slug_uniq`); + await engine.executeRaw(`DROP INDEX IF EXISTS pages_ss_other_name`); + await engine.executeRaw(`DROP INDEX IF EXISTS pages_source_slug_key`); +} + +describe('#550 pages(source_id, slug) unique-index drift', () => { + test('reproduction: dropping the constraint kills put_page; a plain migrate pass heals it', async () => { + await put(); // baseline — fresh brain writes fine + + await dropArbiters(); + expect(put()).rejects.toThrow(/unique or exclusion constraint|ON CONFLICT/i); + + // The brain is stamped at head, so nothing is "pending" — on master this + // pass is a no-op and the brain stays write-dead forever. The #550 fix + // heals on every pass, shape-keyed. + const res = await runMigrations(engine); + expect(res.applied).toBe(0); // proves the heal is NOT a pending migration + + await put(); // writes restored + const status = await checkPagesUniqueIndex(engine); + expect(status.satisfied).toBe(true); + expect(status.indexName).toBe('pages_source_slug_key'); + }); + + test('detection is by columns: a satisfying unique index under a different name counts', async () => { + await dropArbiters(); + await engine.executeRaw( + `CREATE UNIQUE INDEX pages_ss_other_name ON pages(slug, source_id)`, // reversed order also satisfies inference + ); + + const status = await checkPagesUniqueIndex(engine); + expect(status.satisfied).toBe(true); + expect(status.indexName).toBe('pages_ss_other_name'); + + // Repair must be a no-op — no duplicate constraint piled on top. + const r = await repairPagesUniqueIndex(engine); + expect(r.repaired).toBe(false); + expect(r.reason).toBe('already_correct'); + + await put(); // ON CONFLICT (source_id, slug) accepts the reversed-column arbiter + + await dropArbiters(); + await repairPagesUniqueIndex(engine); // restore canonical state + }); + + test('a partial unique index does NOT satisfy ON CONFLICT and is not accepted', async () => { + await dropArbiters(); + await engine.executeRaw( + `CREATE UNIQUE INDEX pages_ss_other_name ON pages(source_id, slug) WHERE slug <> ''`, + ); + + expect(put()).rejects.toThrow(/unique or exclusion constraint|ON CONFLICT/i); + const status = await checkPagesUniqueIndex(engine); + expect(status.satisfied).toBe(false); + + const r = await repairPagesUniqueIndex(engine); + expect(r.repaired).toBe(true); + await put(); + + await engine.executeRaw(`DROP INDEX IF EXISTS pages_ss_other_name`); + }); + + test('canonical name taken by a wrong-shaped index: repair falls back instead of failing', async () => { + await dropArbiters(); + // Correctly named, wrong shape (partial) — must not block the repair. + await engine.executeRaw( + `CREATE UNIQUE INDEX pages_source_slug_key ON pages(source_id, slug) WHERE slug <> ''`, + ); + + const r = await repairPagesUniqueIndex(engine); + expect(r.repaired).toBe(true); + const status = await checkPagesUniqueIndex(engine); + expect(status.satisfied).toBe(true); + expect(status.indexName).toBe('pages_source_slug_uniq'); + await put(); + + // Restore canonical state for later tests. + await dropArbiters(); + await repairPagesUniqueIndex(engine); + }); + + test('repair is idempotent', async () => { + const first = await repairPagesUniqueIndex(engine); + expect(first.reason).toBe('already_correct'); + const second = await repairPagesUniqueIndex(engine); + expect(second.reason).toBe('already_correct'); + }); + + test('doctor check FAILS loudly with the literal repair SQL (and never mentions --force-schema)', async () => { + await dropArbiters(); + + const check = await doctorCheck(engine); + expect(check.name).toBe('pages_unique_index'); + expect(check.status).toBe('fail'); + expect(check.message).toContain(PAGES_UNIQUE_REPAIR_SQL); + expect(check.message).toContain( + 'ALTER TABLE pages ADD CONSTRAINT pages_source_slug_key UNIQUE (source_id, slug);', + ); + expect(check.message).not.toContain('force-schema'); + + await repairPagesUniqueIndex(engine); + const healthy = await doctorCheck(engine); + expect(healthy.status).toBe('ok'); + }); + + test('missing pages table is OK/skip, not FAIL — a fresh brain is not broken', async () => { + const status = await checkPagesUniqueIndex(bare); + expect(status.tablePresent).toBe(false); + expect(status.satisfied).toBe(false); + + const r = await repairPagesUniqueIndex(bare); + expect(r.repaired).toBe(false); + expect(r.reason).toBe('no_table'); + + const check = await doctorCheck(bare); + expect(check.status).toBe('ok'); + }); +});