From f7f8512b1432f1fc3671a4d36ed37bcfbdce5edc Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 7 Jun 2026 08:05:34 -0700 Subject: [PATCH] v0.42.28.0 fix(engine): batch inserts use jsonb_to_recordset, not text[] array literals (#1861) (#1927) * fix(engine): batch inserts use jsonb_to_recordset, not text[] array literals (#1861) addLinksBatch/addTimelineEntriesBatch/addTakesBatch passed free text through unnest(${arr}::text[]); postgres.js serialized it to a Postgres text[] literal that array_in rejected ("malformed array literal") on calendar/Zoom context, aborting the whole `extract links --stale` sweep. Bind the batch as one JSONB doc via jsonb_to_recordset(($1::jsonb)->'rows') through the audited executeRawJsonb contract instead. Shared row builders (src/core/batch-rows.ts) keep both engines byte-identical; NUL is stripped only from free-text body fields (context/summary/detail/claim), while identity/security fields (slugs/source_ids/holder/kind/dates) still reject NUL. addTakesBatch is now batchRetry-wrapped ('addTakesBatch' audit site) and its BrainEngine signature takes BatchOpts. Scalar addLink context is NUL-stripped too. Regression tests on both engines: PGLite always-on poison/NUL/parity suite + DATABASE_URL-gated Postgres lane (the engine that actually crashed). * test: make "no Anthropic key" tests hermetic via withoutAnthropicKey hasAnthropicKey() reads both ANTHROPIC_API_KEY and ~/.gbrain config; tests that only deleted the env var fired a real LLM call on configured machines (warning flipped NO_ANTHROPIC_API_KEY -> LLM_OUTPUT_NOT_JSON). New test/helpers/no-anthropic-key.ts neutralizes both sources (env + GBRAIN_HOME temp dir) for the duration of the call. Refactors the five no-key tests in think-pipeline + takes-mcp-allowlist to use it, including two that previously passed only by luck of the live LLM output. * chore: docs + version bump (v0.42.28.0) KEY_FILES.md/RETRIEVAL.md describe the jsonb_to_recordset batch path; TODOS.md files the #1861 follow-ups (element-isolation, remaining ::text[] sites, shared SQL-string hoist, batch-insert edge-case tests). CHANGELOG + VERSION + package.json to 0.42.28.0. Co-Authored-By: Claude Opus 4.8 (1M context) * docs: sync TESTING.md batch-insert references for v0.42.28.0 The #1861 fix migrated links/timeline/takes batch inserts from unnest(::text[]) to jsonb_to_recordset. Update the stale "postgres-js unnest() binding" note and add the two new poison-regression test files (test/links-timeline-jsonb-poison.test.ts PGLite half, test/e2e/jsonb-batch-poison-postgres.test.ts Postgres lane) to the inventory. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(sql-query): reject top-level array jsonb params in executeRawJsonb (#1861 P2a) The "no top-level array" rule was only a comment. A bare JS array bound to a $N::jsonb position can serialize as a Postgres array literal (not jsonb) through postgres.js, silently re-entering the "malformed array literal" class #1861 just escaped. executeRawJsonb now throws a clear error steering callers to the { rows: [...] } object wrapper. Verified breaks zero call sites (all pass objects or null). Codex adversarial P2a; batch-size enforcement (P2b) filed as a TODO. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 23 ++ TODOS.md | 50 ++++ VERSION | 2 +- docs/TESTING.md | 4 +- docs/architecture/KEY_FILES.md | 4 +- docs/architecture/RETRIEVAL.md | 2 +- package.json | 2 +- src/core/batch-rows.ts | 159 ++++++++++++ src/core/engine.ts | 21 +- src/core/pglite-engine.ts | 118 +++++---- src/core/postgres-engine.ts | 185 +++++++------- src/core/retry.ts | 1 + src/core/sql-query.ts | 21 +- test/core/retry.test.ts | 2 +- test/e2e/jsonb-batch-poison-postgres.test.ts | 103 ++++++++ test/helpers/no-anthropic-key.ts | 41 ++++ test/links-timeline-jsonb-poison.test.ts | 246 +++++++++++++++++++ test/sql-query.test.ts | 16 ++ test/takes-mcp-allowlist.serial.test.ts | 65 +++-- test/think-pipeline.serial.test.ts | 49 ++-- 20 files changed, 872 insertions(+), 242 deletions(-) create mode 100644 src/core/batch-rows.ts create mode 100644 test/e2e/jsonb-batch-poison-postgres.test.ts create mode 100644 test/helpers/no-anthropic-key.ts create mode 100644 test/links-timeline-jsonb-poison.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ef8b61735..b9cb4b2ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ All notable changes to GBrain will be documented in this file. +## [0.42.28.0] - 2026-06-06 + +**`gbrain extract links --stale` no longer dies partway through on calendar and meeting pages.** A full re-extraction sweep (the kind a `LINK_EXTRACTOR_VERSION_TS` bump triggers) used to crash with a Postgres "malformed array literal" error the moment a calendar event's raw text (Zoom links, commas, quotes, braces, em-dashes) landed in a batch. One bad batch aborted the entire run, so the graph never finished reconciling and stale edges couldn't be dropped. The three bulk writers (links, timeline, takes) now pass each batch as a single JSONB document instead of a hand-built `text[]` literal, which encodes arbitrary free text safely. The sweep runs to completion on the messiest brains. + +The same pass makes `addTakesBatch` survive a connection blip mid-run (it retries like the other bulk writers instead of silently dropping the batch) and tightens how stray NUL bytes are handled: junk NULs in free-text bodies (a claim, a meeting summary, a link's context) are stripped so one byte can't abort a batch, while identity fields (slugs, holders, source ids, dates) still reject them. + +Nothing to configure. `gbrain upgrade`, then re-run any extraction that was wedged. + +### Fixed +- `extract links --stale` (and any links/timeline/takes bulk write) no longer crashes with "malformed array literal" when a row carries calendar/meeting free text. Batches bind as one JSONB document via `jsonb_to_recordset` instead of a `text[]` array literal, which also removes the 65535-parameter ceiling on batch size. +- `addTakesBatch` retries on transient connection errors like the other bulk writers, instead of losing the batch on a pooler blip. + +### Changed +- Stray NUL bytes in free-text fields (claim, summary, detail, link context) are stripped before write so a single junk byte can't abort a batch; identity/security fields (slugs, source ids, holders, dates) are left to reject NUL as before. + +### To take advantage of v0.42.28.0 + +Nothing to run. If a `gbrain extract links --stale` sweep previously died on a calendar or meeting page, re-run it: + +```bash +gbrain extract links --source --stale +``` + ## [0.42.26.0] - 2026-06-04 **The Supabase setup docs now match the current dashboard and call out the one thing that actually breaks on IPv4 hosts.** The connection-string instructions were written for the old Supabase UI (two options under Project Settings) and used inconsistent pooler names: some docs said "Connection pooler," others mislabeled port 6543 as the "Session pooler," and a few carried a stale warning to avoid the transaction pooler entirely. The current Supabase UI puts the string under **Connect** in the top navigation with three options (Direct, Transaction pooler, Session pooler). gbrain is tuned for the **Transaction pooler** (port 6543): it disables prepared statements there and routes migrations, DDL, and worker locks to a separate direct connection. This release makes every setup surface say that, consistently. diff --git a/TODOS.md b/TODOS.md index 2e94167dd..2f1c34677 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,55 @@ # TODOS +## gbrain#1861 JSONB batch-insert follow-ups (v0.42+) + +Filed from the #1861 fix (batch inserts migrated from `unnest(${arr}::text[])` to +`jsonb_to_recordset` to stop the "malformed array literal" crash on free-text +context). Deliberately scoped OUT of that PR. See plan + GSTACK REVIEW REPORT at +`~/.claude/plans/system-instruction-you-are-working-velvety-garden.md`. + +- [ ] **P3 — Element-isolation fallback for batch inserts.** On a non-retryable + batch error, retry the batch element-by-element so one bad row can't abort a + 353K-page `extract --stale` sweep, logging the offending `(from_slug, context)` + instead of dying. The durable JSONB fix removed the known crash class (malformed + array literal) and NUL-stripping removed the other known jsonb-parse failure, so + there is no remaining data-dependent crash for this to catch *today* — it's + belt-and-suspenders against unknown future per-row failures. Wire it in + `addLinksBatch`/`addTimelineEntriesBatch`/`addTakesBatch` (or in `batchRetry` as + a post-classification fallback). Issue #1861 option 2. + +- [ ] **P3 — Audit remaining `unnest(${arr}::text[])` write sites.** `setPageAliases` + (alias_norm) and `addCodeEdges` (symbol-qualified names + `metas::jsonb[]`) still + bind through text-array literals. They carry normalized identifiers / symbol names, + not free prose, so the crash risk is far lower than calendar context — but they are + the same bug class and a hostile alias/symbol (or an embedded NUL) could still trip + them. Migrate to `jsonb_to_recordset` via the shared `batch-rows.ts` pattern if/when + one is observed failing, or proactively for completeness. `markPagesExtractedBatch` + is NOT in this set (slugs/source-ids/timestamps only — no free text). + + +- [ ] **P3 — Single-source the batch INSERT SQL strings.** After #1861 the + links/timeline/takes `INSERT ... jsonb_to_recordset(($1::jsonb)->'rows')` SQL is + byte-identical between `postgres-engine.ts` and `pglite-engine.ts` (row builders already + hoisted to `batch-rows.ts`, but the SQL text is still duplicated). Hoist the three SQL + strings into exported constants in `batch-rows.ts` so a recordset column added to one + engine can't silently drift from the other. `test/e2e/engine-parity.test.ts` pins + behavior; a shared constant prevents drift at edit time. (Maintainability specialist.) + +- [ ] **P3 — Backfill batch-insert edge-case tests.** Edges sharing already-covered helper + code but lacking direct assertions: (a) `addTakesBatch` retries on an injected retryable + error + AbortSignal aborts (the `batchRetry` wrap is proven for links/timeline; takes + inherits the identical wrapper but isn't exercised directly); (b) `addTakesBatch` + intra-batch duplicate `(page_id,row_num)` rejects under `ON CONFLICT DO UPDATE` + (comment-claimed, unasserted). (Testing specialist.) + +- [ ] **P3 — Enforce a max batch size on the JSONB bulk inserts.** One JSONB datum + is not unbounded (server-side parse/memory ceiling). In-tree callers chunk well + under any limit (extract ~100, NER ~500), and `batch-rows.ts` documents "chunk + ~1-5K rows", but nothing enforces it for an external direct-engine caller passing + a giant batch. Consider a `BATCH_INSERT_MAX` constant + a clear throw, mirroring + the existing `DELETE_BATCH_SIZE` valve in `deletePages`. Deferred because no + in-tree caller hits it and the cap value is a judgment call. (Codex #1861 P2b.) + ## v0.42.21.0 module-singleton ownership follow-ups (v0.42+) Filed from the v0.42.21.0 wave (#1404/#1471/#1619 — the dream-cycle diff --git a/VERSION b/VERSION index b8bc4029e..83fabc494 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.26.0 \ No newline at end of file +0.42.28.0 diff --git a/docs/TESTING.md b/docs/TESTING.md index 545b61bf9..84c05e39b 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -141,6 +141,7 @@ Unit tests and what they cover: - `test/yaml-lite.test.ts` — YAML parsing. - `test/check-update.test.ts` — version check + update CLI. - `test/pglite-engine.test.ts` — PGLite engine, all BrainEngine methods including `addLinksBatch` / `addTimelineEntriesBatch` (empty batch, missing optionals, within-batch dedup via ON CONFLICT, missing-slug rows dropped by JOIN, half-existing batch, batch of 100) plus `connect()` error-wrap assertion (original error nested, #223 link in message, lock released). +- `test/links-timeline-jsonb-poison.test.ts` — gbrain#1861 PGLite half (always-on, no `DATABASE_URL`). Locks the `jsonb_to_recordset` batch-insert path for links/timeline/takes against free-text "poison" payloads (commas, quotes, backslashes, braces, em-dashes) and asserts NUL is stripped from free-text body fields but rejected in identity fields. The Postgres lane (`test/e2e/jsonb-batch-poison-postgres.test.ts`) is the one that actually reproduced the original crash. - `test/engine-factory.test.ts` — engine factory + dynamic imports. - `test/integrations.test.ts` — recipe parsing, CLI routing, recipe validation. - `test/publish.test.ts` — content stripping, encryption, password generation, HTML output. @@ -208,9 +209,10 @@ Unit tests and what they cover: E2E tests live in `test/e2e/` and run against real Postgres+pgvector (require `DATABASE_URL`), except where noted as PGLite in-memory (no `DATABASE_URL` needed). -- `bun run test:e2e` runs Tier 1 (mechanical, all operations, no API keys). Includes dedicated cases for the postgres-engine `addLinksBatch` / `addTimelineEntriesBatch` bind path — postgres-js's `unnest()` binding is structurally different from PGLite's and gets its own coverage. +- `bun run test:e2e` runs Tier 1 (mechanical, all operations, no API keys). Includes dedicated cases for the postgres-engine `addLinksBatch` / `addTimelineEntriesBatch` bind path — postgres-js's JSONB bind (`jsonb_to_recordset(($1::jsonb)->'rows')`) differs from PGLite's and gets its own coverage. - `test/e2e/search-quality.test.ts` — search quality against PGLite (no API keys, in-memory). - `test/e2e/graph-quality.test.ts` — knowledge graph pipeline (auto-link via put_page, reconciliation, traversePaths) against PGLite in-memory. +- `test/e2e/jsonb-batch-poison-postgres.test.ts` — gbrain#1861 regression, the engine that actually crashed. Seeds free-text "poison" context (Zoom URL with `?pwd=`, commas, quotes, Windows backslash path, braces, em-dash) and asserts the links/timeline/takes batch writers no longer error with "malformed array literal"; also asserts NUL is stripped from free-text bodies (`context`/`summary`/`detail`/`claim`) and still rejected in identity fields. `DATABASE_URL`-gated. - `test/e2e/postgres-jsonb.test.ts` — round-trips all 5 JSONB write sites (`pages.frontmatter`, `raw_data.data`, `ingest_log.pages_updated`, `files.metadata`, `page_versions.frontmatter`) against real Postgres and asserts `jsonb_typeof='object'` plus `->>'key'` returns the expected scalar. Guards against the double-encode bug. - `test/e2e/integrity-batch.test.ts` — parity for `scanIntegrity`'s batch-load fast path vs sequential. Cases (dedup, hits, validate, topPages) seed a fixture and assert both paths return identical results. Dedup case uses raw SQL via `getConn().unsafe()` to seed a `(test-source-2, people/alice)` row alongside the default-source row, since `engine.putPage` doesn't take a `source_id`. Pins multi-source overcounting; the "multi-source duplicate slugs scan once" case expects both batch + sequential paths to report 2. - `test/e2e/jsonb-roundtrip.test.ts` — companion regression against the 4 doctor-scanned JSONB sites. Assertion-level overlap with `postgres-jsonb.test.ts` is intentional defense-in-depth: if doctor's scan surface drifts from the actual write surface, one of these tests catches it. diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index f6fca7085..1cebee185 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -21,8 +21,8 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/search.ts:gbrain search stats` extension — `graph_signals` section (enabled/source/failures_count/failures_by_reason). JSON envelope adds a `graph_signals` sibling property; `_meta.metric_glossary` adds `graph_signals.enabled` + `graph_signals.failures_by_reason`. Human output prints the section after the existing block. Reads `search.graph_signals` config first, falls back to the mode default. Pinned by `test/search/search-stats-graph-signals.test.ts`. - `src/commands/doctor.ts` extension — `graph_signals_coverage` check wired into both `runDoctor` (local) and `doctorReportRemote` (HTTP/JSON thin-client path). Reads `search.graph_signals` config first, falls back to mode default; silent `ok` when disabled. Computes inbound link coverage on the page set; warns at <10% with `gbrain extract all` fix hint; `ok` at ≥30% ("fire on most queries") and 10-29% ("fire occasionally"), each with the percentage embedded. Pinned by cases in `test/doctor.test.ts`. - `src/core/engine-factory.ts` — Engine factory with dynamic imports (`'pglite'` | `'postgres'`). -- `src/core/pglite-engine.ts` — PGLite (embedded Postgres 17.5 via WASM) implementation, all 40 BrainEngine methods. `addLinksBatch`/`addTimelineEntriesBatch` use multi-row `unnest()` with manual `$N` placeholders. `connect()` wraps `PGlite.create()` in a try/catch that emits an actionable error (macOS 26.3 WASM bug #223, points at `gbrain doctor`); the lock is released on failure so the next process can retry cleanly. `searchKeyword`/`searchKeywordChunks` multiply `ts_rank` by the source-factor CASE at chunk grain; `searchVector` is a two-stage CTE — inner CTE keeps `ORDER BY cc.embedding <=> vec` so HNSW stays usable, outer SELECT re-ranks by `raw_score * source_factor`, inner LIMIT scales with offset to preserve pagination. `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL — probes for forward-referenced state the embedded blob needs (`pages.source_id`, `links.link_source`, `links.origin_page_id`, `content_chunks.symbol_name`, `content_chunks.language`, `sources` FK target, plus `files.source_id`, `files.page_id`, `oauth_clients.source_id`, `oauth_clients.federated_read`, `sources.archived`, `sources.archived_at`, `sources.archive_expires_at`) and adds only what's missing; threads the DDL connection from `initSchema` so probes run inside the advisory-lock scope; no-op on fresh installs and modern brains (closes the upgrade-wedge bug class #239/#243/#266/#357/#366/#374/#375/#378/#395/#396/#1018/#974/#820). `getBrainScore` returns 100/100 with full breakdown (35/25/15/15/10) when `pageCount === 0` (vacuous truth — empty brain has no coverage problem); Pinned by `test/brain-score-breakdown.test.ts` empty-brain assertion + `test/doctor-report-remote.serial.test.ts`. `disconnect()` uses snapshot+early-null (snapshot `_db`/`_lock`, null instance fields BEFORE any `await` so a concurrent `connect()` can't see a partial mid-close state) wrapped in try/finally guaranteeing lock-release even if `db.close()` throws; KEEPS close-then-release order (release-then-close was rejected: it would widen the window where a sibling process connects to a still-closing brain); Pinned by `test/pglite-engine-disconnect.serial.test.ts`. Exports `classifyPgliteInitError(message): 'bunfs' | 'macos-26-3' | 'unknown'` + `buildPgliteInitErrorMessage(verdict, original)` routing the catch-block hint by failure shape (`bunfs` matches literal `$$bunfs` OR `ENOENT[\s\S]*pglite\.data` co-occurrence, surfaces a paste-ready `bun upgrade` + Node fallback; `macos-26-3` keeps the #223 link; `unknown` falls through); Pinned by `test/pglite-init-classifier.test.ts`. Implements `deletePages(slugs, {sourceId})` + `resolveSlugsByPaths(paths, {sourceId})` via `slug = ANY($1::text[])` array-param binding, caller-chunking primitive throwing when input exceeds `DELETE_BATCH_SIZE`, `deletePages` returns `RETURNING slug` rows so callers filter `pagesAffected` to confirmed deletes. Implements the embedding-signature stale-detection quartet — `sumStaleChunkChars({sourceId?, signature?})`, `setPageEmbeddingSignature(slug, {sourceId?, signature})`, `invalidateStaleSignatureEmbeddings({signature, sourceId?})`, widened `countStaleChunks({sourceId?, signature?})` (the `signature` opt widens via `JOIN pages p ... WHERE cc.embedding IS NULL OR (p.embedding_signature IS NOT NULL AND p.embedding_signature <> $signature)`, NULL grandfathered); parity SQL with postgres-engine.ts. PGLite-specific DDL (pgvector, pg_trgm, triggers). -- `src/core/postgres-engine.ts` — Postgres + pgvector implementation (Supabase / self-hosted). `addLinksBatch`/`addTimelineEntriesBatch` use `INSERT ... SELECT FROM unnest($1::text[], ...) JOIN pages ON CONFLICT DO NOTHING RETURNING 1` — 4-5 array params regardless of batch size, sidesteps the 65535-parameter cap. `searchKeyword`/`searchVector` scope `statement_timeout` via `sql.begin` + `SET LOCAL` so the GUC dies with the transaction instead of leaking across the pooled postgres.js connection. `getEmbeddingsByChunkIds` uses `tryParseEmbedding` so one corrupt row skips+warns instead of killing the query. `searchKeyword`/`searchKeywordChunks`/`searchVector` apply source-aware ranking by inlining the source-factor CASE and `NOT (col LIKE …)` hard-exclude from `src/core/search/sql-ranking.ts`; `searchVector` is a two-stage CTE (HNSW-safe inner ORDER BY, source-boost re-rank in outer SELECT) carrying `p.source_id` inner→outer. `_savedConfig` retains the connect config; `reconnect()` tears down + recreates the pool from saved config (called by supervisor watchdog after 3 consecutive health-check failures, and by `batchRetry` on a retryable connection error). Concurrent callers share one in-flight `_reconnectPromise` (they await the single reconnect rather than racing a half-rebuilt pool); ownership re-samples through the atomic `db.connect()` token on the connect leg. `reconnect(ctx?)` accepts the triggering error and records a pool-recovery audit event (`reap_detected`/`reconnect_other`/`reconnect_succeeded`/`reconnect_failed`) for the `pool_reap_health` doctor check. `executeRaw` is a single-statement passthrough — no per-call retry (unsound for non-idempotent statements; recovery is supervisor-driven). `connect()` applies `resolveSessionTimeouts()` from `db.ts` as connection-time startup parameters (`statement_timeout`, `idle_in_transaction_session_timeout`) so orphan pgbouncer backends can't hold locks for hours. `countStaleChunks()`+`listStaleChunks()` server-side-filter on `embedding IS NULL` for `embed --stale` (eliminates ~76 MB/call client-side pull); `upsertChunks()` resets both `embedding` AND `embedded_at` to NULL when chunk_text changes without a new embedding. `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL on the same probe set as PGLite (extended for column-only forward-reference cases: `files.source_id`, `files.page_id`, `oauth_clients.source_id`, `oauth_clients.federated_read`, `sources.archived`/`archived_at`/`archive_expires_at`); the entire probe path runs on the DDL connection threaded from `initSchema` (closing a concurrent-bootstrap race for Supabase pooler users); closes #1018/#974/#820. `disconnect()` is idempotent — `_connectionStyle` tracks whether the engine owns its pool (worker engines) or shares the module-level singleton; second call on an instance-pool engine is a no-op rather than clobbering the singleton; and a module-style engine only calls `db.disconnect()` when it owns the singleton (`_ownsModuleSingleton`, set from the `db.connect()` creation token), so a borrower probe engine's teardown leaves the cycle owner's connection intact. Pinned by `test/e2e/postgres-engine-disconnect-idempotency.test.ts` + `test/postgres-engine-singleton-ownership.test.ts`. `getBrainScore` empty-brain parity with PGLite — 100/100 with breakdown 35/25/15/15/10 when `pageCount === 0` (both engines must agree to keep `doctor-report-remote.serial.test.ts` deterministic). Implements `deletePages(slugs, {sourceId}): Promise` via `DELETE FROM pages WHERE slug = ANY($1::text[]) AND source_id = $2 RETURNING slug` (single round-trip; caller chunks); `resolveSlugsByPaths` does `SELECT slug, source_path FROM pages WHERE source_path = ANY($1::text[]) AND source_id = $2`; FK cascades through `content_chunks`/`links`/`tags`/`raw_data`/`timeline_entries`/`page_versions`, `files.page_id`+`links.origin_page_id` go SET NULL; throws when input exceeds `DELETE_BATCH_SIZE` (from `src/core/engine-constants.ts`); both short-circuit on empty input. Implements the embedding-signature stale-detection quartet (`sumStaleChunkChars`, `setPageEmbeddingSignature`, `invalidateStaleSignatureEmbeddings`, widened `countStaleChunks`, all accept optional `signature` extending "stale" to model/dims-swap drift via the `pages.embedding_signature` JOIN, NULL grandfathered; the `embedding IS NULL` server-side filter is preserved as the no-signature fast path); Pinned by `test/e2e/engine-parity.test.ts`. +- `src/core/pglite-engine.ts` — PGLite (embedded Postgres 17.5 via WASM) implementation, all 40 BrainEngine methods. `addLinksBatch`/`addTimelineEntriesBatch`/`addTakesBatch` pass the whole batch as one JSONB document via `jsonb_to_recordset(($1::jsonb)->'rows')` (bound through `executeRawJsonb` with a `{ rows }` wrapper; rows built by the shared `src/core/batch-rows.ts` helpers, NUL-stripped), and are `batchRetry`-wrapped. `connect()` wraps `PGlite.create()` in a try/catch that emits an actionable error (macOS 26.3 WASM bug #223, points at `gbrain doctor`); the lock is released on failure so the next process can retry cleanly. `searchKeyword`/`searchKeywordChunks` multiply `ts_rank` by the source-factor CASE at chunk grain; `searchVector` is a two-stage CTE — inner CTE keeps `ORDER BY cc.embedding <=> vec` so HNSW stays usable, outer SELECT re-ranks by `raw_score * source_factor`, inner LIMIT scales with offset to preserve pagination. `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL — probes for forward-referenced state the embedded blob needs (`pages.source_id`, `links.link_source`, `links.origin_page_id`, `content_chunks.symbol_name`, `content_chunks.language`, `sources` FK target, plus `files.source_id`, `files.page_id`, `oauth_clients.source_id`, `oauth_clients.federated_read`, `sources.archived`, `sources.archived_at`, `sources.archive_expires_at`) and adds only what's missing; threads the DDL connection from `initSchema` so probes run inside the advisory-lock scope; no-op on fresh installs and modern brains (closes the upgrade-wedge bug class #239/#243/#266/#357/#366/#374/#375/#378/#395/#396/#1018/#974/#820). `getBrainScore` returns 100/100 with full breakdown (35/25/15/15/10) when `pageCount === 0` (vacuous truth — empty brain has no coverage problem); Pinned by `test/brain-score-breakdown.test.ts` empty-brain assertion + `test/doctor-report-remote.serial.test.ts`. `disconnect()` uses snapshot+early-null (snapshot `_db`/`_lock`, null instance fields BEFORE any `await` so a concurrent `connect()` can't see a partial mid-close state) wrapped in try/finally guaranteeing lock-release even if `db.close()` throws; KEEPS close-then-release order (release-then-close was rejected: it would widen the window where a sibling process connects to a still-closing brain); Pinned by `test/pglite-engine-disconnect.serial.test.ts`. Exports `classifyPgliteInitError(message): 'bunfs' | 'macos-26-3' | 'unknown'` + `buildPgliteInitErrorMessage(verdict, original)` routing the catch-block hint by failure shape (`bunfs` matches literal `$$bunfs` OR `ENOENT[\s\S]*pglite\.data` co-occurrence, surfaces a paste-ready `bun upgrade` + Node fallback; `macos-26-3` keeps the #223 link; `unknown` falls through); Pinned by `test/pglite-init-classifier.test.ts`. Implements `deletePages(slugs, {sourceId})` + `resolveSlugsByPaths(paths, {sourceId})` via `slug = ANY($1::text[])` array-param binding, caller-chunking primitive throwing when input exceeds `DELETE_BATCH_SIZE`, `deletePages` returns `RETURNING slug` rows so callers filter `pagesAffected` to confirmed deletes. Implements the embedding-signature stale-detection quartet — `sumStaleChunkChars({sourceId?, signature?})`, `setPageEmbeddingSignature(slug, {sourceId?, signature})`, `invalidateStaleSignatureEmbeddings({signature, sourceId?})`, widened `countStaleChunks({sourceId?, signature?})` (the `signature` opt widens via `JOIN pages p ... WHERE cc.embedding IS NULL OR (p.embedding_signature IS NOT NULL AND p.embedding_signature <> $signature)`, NULL grandfathered); parity SQL with postgres-engine.ts. PGLite-specific DDL (pgvector, pg_trgm, triggers). +- `src/core/postgres-engine.ts` — Postgres + pgvector implementation (Supabase / self-hosted). `addLinksBatch`/`addTimelineEntriesBatch`/`addTakesBatch` pass the batch as one JSONB document — `INSERT ... SELECT FROM jsonb_to_recordset(($1::jsonb)->'rows') AS v(...) JOIN pages ...` bound through `executeRawJsonb({ rows })` — which encodes arbitrary free text safely (the old `unnest(${arr}::text[])` array-literal path crashed Postgres with "malformed array literal" on calendar/Zoom context, gbrain#1861) and sidesteps the 65535-parameter cap; takes declares native recordset column types (`page_id int, weight real, active boolean, …`) so no per-element casts; all three are `batchRetry`-wrapped. `searchKeyword`/`searchVector` scope `statement_timeout` via `sql.begin` + `SET LOCAL` so the GUC dies with the transaction instead of leaking across the pooled postgres.js connection. `getEmbeddingsByChunkIds` uses `tryParseEmbedding` so one corrupt row skips+warns instead of killing the query. `searchKeyword`/`searchKeywordChunks`/`searchVector` apply source-aware ranking by inlining the source-factor CASE and `NOT (col LIKE …)` hard-exclude from `src/core/search/sql-ranking.ts`; `searchVector` is a two-stage CTE (HNSW-safe inner ORDER BY, source-boost re-rank in outer SELECT) carrying `p.source_id` inner→outer. `_savedConfig` retains the connect config; `reconnect()` tears down + recreates the pool from saved config (called by supervisor watchdog after 3 consecutive health-check failures, and by `batchRetry` on a retryable connection error). Concurrent callers share one in-flight `_reconnectPromise` (they await the single reconnect rather than racing a half-rebuilt pool); ownership re-samples through the atomic `db.connect()` token on the connect leg. `reconnect(ctx?)` accepts the triggering error and records a pool-recovery audit event (`reap_detected`/`reconnect_other`/`reconnect_succeeded`/`reconnect_failed`) for the `pool_reap_health` doctor check. `executeRaw` is a single-statement passthrough — no per-call retry (unsound for non-idempotent statements; recovery is supervisor-driven). `connect()` applies `resolveSessionTimeouts()` from `db.ts` as connection-time startup parameters (`statement_timeout`, `idle_in_transaction_session_timeout`) so orphan pgbouncer backends can't hold locks for hours. `countStaleChunks()`+`listStaleChunks()` server-side-filter on `embedding IS NULL` for `embed --stale` (eliminates ~76 MB/call client-side pull); `upsertChunks()` resets both `embedding` AND `embedded_at` to NULL when chunk_text changes without a new embedding. `initSchema()` calls `applyForwardReferenceBootstrap()` BEFORE replaying SCHEMA_SQL on the same probe set as PGLite (extended for column-only forward-reference cases: `files.source_id`, `files.page_id`, `oauth_clients.source_id`, `oauth_clients.federated_read`, `sources.archived`/`archived_at`/`archive_expires_at`); the entire probe path runs on the DDL connection threaded from `initSchema` (closing a concurrent-bootstrap race for Supabase pooler users); closes #1018/#974/#820. `disconnect()` is idempotent — `_connectionStyle` tracks whether the engine owns its pool (worker engines) or shares the module-level singleton; second call on an instance-pool engine is a no-op rather than clobbering the singleton; and a module-style engine only calls `db.disconnect()` when it owns the singleton (`_ownsModuleSingleton`, set from the `db.connect()` creation token), so a borrower probe engine's teardown leaves the cycle owner's connection intact. Pinned by `test/e2e/postgres-engine-disconnect-idempotency.test.ts` + `test/postgres-engine-singleton-ownership.test.ts`. `getBrainScore` empty-brain parity with PGLite — 100/100 with breakdown 35/25/15/15/10 when `pageCount === 0` (both engines must agree to keep `doctor-report-remote.serial.test.ts` deterministic). Implements `deletePages(slugs, {sourceId}): Promise` via `DELETE FROM pages WHERE slug = ANY($1::text[]) AND source_id = $2 RETURNING slug` (single round-trip; caller chunks); `resolveSlugsByPaths` does `SELECT slug, source_path FROM pages WHERE source_path = ANY($1::text[]) AND source_id = $2`; FK cascades through `content_chunks`/`links`/`tags`/`raw_data`/`timeline_entries`/`page_versions`, `files.page_id`+`links.origin_page_id` go SET NULL; throws when input exceeds `DELETE_BATCH_SIZE` (from `src/core/engine-constants.ts`); both short-circuit on empty input. Implements the embedding-signature stale-detection quartet (`sumStaleChunkChars`, `setPageEmbeddingSignature`, `invalidateStaleSignatureEmbeddings`, widened `countStaleChunks`, all accept optional `signature` extending "stale" to model/dims-swap drift via the `pages.embedding_signature` JOIN, NULL grandfathered; the `embedding IS NULL` server-side filter is preserved as the no-signature fast path); Pinned by `test/e2e/engine-parity.test.ts`. - `src/core/cjk.ts` — Single source of truth for CJK detection. Exports `CJK_RANGES_REGEX`, `CJK_SLUG_CHARS` (character-class fragment for embedding inside other regexes), `CJK_SENTENCE_DELIMITERS` (`。！？`), `CJK_CLAUSE_DELIMITERS` (`；：，、`), `CJK_DENSITY_THRESHOLD = 0.30`, `hasCJK(s)`, `countCJKAwareWords(s)` (30% density threshold — English docs with one Japanese term stay whitespace-tokenized; Chinese-dominant docs get char-counted), and `escapeLikePattern(s)` (escapes `%`, `_`, `\\` for `ILIKE ... ESCAPE '\\'`). BMP-only ranges (Han / Hiragana / Katakana / Hangul Syllables). Consumers: `expansion.ts`, `sync.ts:slugifySegment`, `operations.ts:validatePageSlug + validateFilename`, `chunkers/recursive.ts:countWords + DELIMITERS`, `pglite-engine.ts:searchKeyword + searchKeywordChunks`. - `src/core/audit-slug-fallback.ts` — Weekly ISO-week-rotated audit JSONL at `~/.gbrain/audit/slug-fallback-YYYY-Www.jsonl`. `logSlugFallback(slug, sourcePath)` fires when `importFromFile` falls back to a frontmatter slug because `slugifyPath` returned empty (emoji / Thai / Arabic / non-CJK exotic-script filenames). `readRecentSlugFallbacks(days)` reads the last N days for `gbrain doctor`'s `slug_fallback_audit` check. Honors `GBRAIN_AUDIT_DIR` via the shared `resolveAuditDir()`. Separate surface from `sync-failures.jsonl` — that file carries bookmark-gating semantics that info events shouldn't trigger. - `src/core/embedding-pricing.ts` — `EMBEDDING_PRICING` map keyed `provider:model` for the post-upgrade reindex cost estimate. Sibling to `anthropic-pricing.ts`. Entries: OpenAI text-embedding-3-large ($0.13/1M), 3-small ($0.02/1M), ada-002 ($0.10/1M), Voyage 3-large ($0.18/1M), 3 ($0.06/1M). `lookupEmbeddingPrice(modelString)` returns a tagged union (`known` with price + `unknown` with provider name); `estimateCostFromChars(charCount, pricePerMTok)` uses 3.5 chars/token. Unknown providers degrade to "estimate unavailable" instead of fabricating numbers. diff --git a/docs/architecture/RETRIEVAL.md b/docs/architecture/RETRIEVAL.md index 6193aa7ac..82d0e2a98 100644 --- a/docs/architecture/RETRIEVAL.md +++ b/docs/architecture/RETRIEVAL.md @@ -40,7 +40,7 @@ Every `put_page` runs `extractEntityRefs` on the markdown body. It matches: - Obsidian wikilinks: `[[wiki/people/garry-tan|Garry Tan]]` - Typed-link blockquotes: `> **Convention:** see [path](path).` -Three regexes, zero LLM tokens, single SQL `addLinksBatch` call with `INSERT ... SELECT FROM unnest(...) JOIN pages ON CONFLICT DO NOTHING RETURNING 1`. The graph grows on every write at near-zero cost. On a 17K-page brain, full graph extract completes in seconds. +Three regexes, zero LLM tokens, single SQL `addLinksBatch` call with `INSERT ... SELECT FROM jsonb_to_recordset(($1::jsonb)->'rows') JOIN pages ON CONFLICT DO NOTHING RETURNING 1` (free-text-safe; the prior `unnest(${arr}::text[])` form crashed on calendar/Zoom context per gbrain#1861). The graph grows on every write at near-zero cost. On a 17K-page brain, full graph extract completes in seconds. Heuristic link-type inference (`attended`, `works_at`, `invested_in`, `founded`, `advises`) fires from surrounding sentence context — also LLM-free. Power users who want richer types add them via the typed-link blockquote convention. diff --git a/package.json b/package.json index d438ec6f2..df1529f1a 100644 --- a/package.json +++ b/package.json @@ -143,5 +143,5 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.26.0" + "version": "0.42.28.0" } diff --git a/src/core/batch-rows.ts b/src/core/batch-rows.ts new file mode 100644 index 000000000..c1b346c1c --- /dev/null +++ b/src/core/batch-rows.ts @@ -0,0 +1,159 @@ +/** + * Shared batch-insert row builders (gbrain#1861). + * + * WHY THIS FILE EXISTS + * -------------------- + * `addLinksBatch` / `addTimelineEntriesBatch` / `addTakesBatch` used to bind + * free text through `unnest(${arr}::text[])`. postgres.js serializes a JS + * string[] into a Postgres `text[]` literal (`{"...","..."}`); calendar/Zoom + * context strings (commas, quotes, braces, em-dashes) produced a literal that + * Postgres `array_in` rejected -> "malformed array literal", which aborted the + * whole `extract links --stale` sweep. The fix passes the batch as a single + * JSONB document via `jsonb_to_recordset((($1::jsonb)->'rows'))`, which encodes + * arbitrary free text safely and dodges the 65535-bind-param cap. + * + * Both engines (postgres.js and PGLite) must build the SAME row objects or they + * drift, so the object construction lives here once and both engines import it. + * + * LinkBatchInput[] ---+ + * TimelineInput[] ----+--> build*Rows() --> [{...}, ...] --> { rows } wrapper + * TakeBatchInput[] ---+ | | + * stripNul free-text executeRawJsonb + * fields only $1::jsonb -> 'rows' + * jsonb_to_recordset(...) + * + * NUL POLICY (codex P0 hardening): Postgres `jsonb` rejects the Unicode NUL + * escape, and Postgres `text` cannot store a NUL either, so the OLD + * `unnest(::text[])` path rejected (errored) any row carrying an embedded NUL. + * We deliberately PRESERVE that reject semantics for IDENTITY and + * security-relevant fields: slugs, source_ids, `holder`, `kind`, dates, and the + * enum-ish `link_type` / `link_source` / `origin_slug` / `origin_field`. Those + * are left UN-stripped, so a NUL in them still errors the batch and can never + * silently retarget a row to a different page/source or normalize a `holder` + * past the read-side `holder = ANY(allowlist)` privacy filter. + * + * `stripNul` is applied ONLY to genuinely free-prose body fields where a junk + * NUL plausibly arrives from calendar/meeting/LLM content and where dropping the + * whole batch would be the worse outcome: `context` (links), `summary` + `detail` + * (timeline), `claim` (takes). NUL is the ONLY character ever stripped; commas, + * quotes, braces, and em-dashes are exactly what JSONB encodes correctly, and + * stripping them would corrupt user data. + * + * DEFAULTING NOTE: the builders reproduce each method's exact pre-#1861 + * defaulting. `|| ''` / `|| 'markdown'` / `|| 'default'` collapse empty strings; + * `origin_slug` / `origin_field` use truthy-`|| null` (empty string -> null, + * which the LEFT JOIN treats as no-match); `link_kind` uses `?? null` (empty + * string preserved). Do NOT "simplify" `||` to `??`; it changes empty-string + * behavior. + * + * BATCH SIZE: one JSONB parameter dodges the 65535-param cap but is not + * unbounded; it has a server-side datum/parse-memory ceiling. In-tree callers + * batch small (extract links ~100/batch, NER ~500), well within budget. Direct + * engine callers passing arbitrarily large batches should chunk (~1-5K rows). + */ + +import type { LinkBatchInput, TimelineBatchInput, TakeBatchInput } from './engine.ts'; +import { normalizeWeightForStorage } from './takes-fence.ts'; + +/** + * Strip Unicode NUL (U+0000) from a free-text body field. Fast-path the common + * case (no NUL) so the regex replace only runs when a NUL is actually present. + * Only call this on free-prose columns, never on identity/security fields (see + * the NUL POLICY note above). + */ +export const stripNul = (s: string): string => (s.includes('\0') ? s.replace(/\0/g, '') : s); + +/** One links row, keys === the jsonb_to_recordset column list. */ +export interface LinkRow { + from_slug: string; + to_slug: string; + link_type: string; + context: string; + link_source: string; + origin_slug: string | null; + origin_field: string | null; + from_source_id: string; + to_source_id: string; + origin_source_id: string; + link_kind: string | null; +} + +/** One timeline row, keys === the jsonb_to_recordset column list. */ +export interface TimelineRow { + slug: string; + date: string; + source: string; + summary: string; + detail: string; + source_id: string; +} + +/** One takes row, keys === the jsonb_to_recordset column list. Numbers/booleans + * stay JSON-native so the recordset can declare native column types. */ +export interface TakeRow { + page_id: number; + row_num: number; + claim: string; + kind: string; + holder: string; + weight: number; + since_date: string | null; + until_date: string | null; + source: string | null; + superseded_by: number | null; + active: boolean; +} + +export function buildLinkRows(links: LinkBatchInput[]): LinkRow[] { + return links.map(l => ({ + from_slug: l.from_slug, + to_slug: l.to_slug, + link_type: l.link_type || '', + context: stripNul(l.context || ''), // free-text body: NUL-stripped + link_source: l.link_source || 'markdown', + origin_slug: l.origin_slug || null, + origin_field: l.origin_field || null, + from_source_id: l.from_source_id || 'default', + to_source_id: l.to_source_id || 'default', + origin_source_id: l.origin_source_id || 'default', + link_kind: l.link_kind ?? null, + })); +} + +export function buildTimelineRows(entries: TimelineBatchInput[]): TimelineRow[] { + return entries.map(e => ({ + slug: e.slug, + date: e.date, + source: e.source || '', + summary: stripNul(e.summary), // free-text body: NUL-stripped + detail: stripNul(e.detail || ''), // free-text body: NUL-stripped + source_id: e.source_id || 'default', + })); +} + +/** + * Build takes rows AND report how many weights were clamped, so the caller can + * emit the TAKES_WEIGHT_CLAMPED stderr counter exactly as before. Weight + * normalization (clamp to [0,1] + round to 0.05 grid) stays centralized here. + */ +export function buildTakeRows(rowsIn: TakeBatchInput[]): { rows: TakeRow[]; weightClamped: number } { + let weightClamped = 0; + const rows = rowsIn.map(r => { + const { weight, clamped } = normalizeWeightForStorage(r.weight); + if (clamped) weightClamped++; + return { + page_id: r.page_id, + row_num: r.row_num, + claim: stripNul(r.claim), // free-text body: NUL-stripped + kind: r.kind, + holder: r.holder, + weight, + since_date: r.since_date ?? null, + until_date: r.until_date ?? null, + source: r.source ?? null, + superseded_by: r.superseded_by ?? null, + active: r.active ?? true, + }; + }); + return { rows, weightClamped }; +} diff --git a/src/core/engine.ts b/src/core/engine.ts index 4d52f3b3e..a7e864faa 100644 --- a/src/core/engine.ts +++ b/src/core/engine.ts @@ -98,7 +98,7 @@ export interface FileSpec { /** * v0.41.18.0 — shared opts for engine batch primitives that self-retry on * transient connection errors. Threaded through addLinksBatch / - * addTimelineEntriesBatch / upsertChunks. + * addTimelineEntriesBatch / addTakesBatch / upsertChunks. * * Retry semantics: each batch primitive wraps its internal SQL in * `withRetry(BULK_RETRY_OPTS)` (default `{maxRetries:3, delayMs:1000, @@ -1056,7 +1056,8 @@ export interface BrainEngine { }): Promise; /** * Stamp `links_extracted_at` for a batch of pages keyed on the unique - * `(slug, source_id)` pair (unnest idiom, mirrors addLinksBatch). + * `(slug, source_id)` pair (3-array `unnest` idiom; slugs/ids/timestamps only, + * so unlike the free-text batch inserts it never needed the #1861 jsonb migration). * Short-circuits on empty input. Called AFTER the link/timeline flush so a * crash mid-batch leaves pages unstamped and they re-extract next run. * @@ -1342,17 +1343,23 @@ export interface BrainEngine { // v0.28: Takes (typed/weighted/attributed claims) + synthesis evidence // ============================================================ /** - * Bulk insert/upsert takes. Uses `unnest()` (Postgres) or manual `$N` - * placeholders (PGLite). Idempotency: ON CONFLICT (page_id, row_num) DO UPDATE - * — re-extract on a changed claim/weight updates the row in place. - * Returns the number of rows inserted OR updated. + * Bulk insert/upsert takes. Binds the whole batch as one JSONB document via + * `jsonb_to_recordset(($1::jsonb)->'rows')` through `executeRawJsonb` (#1861; + * free-text-safe, replaced the prior `unnest(::text[])` path). Idempotency: + * ON CONFLICT (page_id, row_num) DO UPDATE — re-extract on a changed + * claim/weight updates the row in place. Returns the number of rows inserted + * OR updated. Row construction + weight clamp/round + NUL-strip live in + * `src/core/batch-rows.ts:buildTakeRows` (shared across both engines). + * + * Wrapped in `batchRetry` like the other batch primitives, so `opts` (auditSite, + * AbortSignal) is honored; same no-double-wrap contract as `BatchOpts`. * * Weight outside [0, 1] is clamped server-side and surfaces a stderr * warning per call (`TAKES_WEIGHT_CLAMPED`). Invalid `kind` values * fail the whole batch via the CHECK constraint — caller is responsible * for parser validation upstream. */ - addTakesBatch(rows: TakeBatchInput[]): Promise; + addTakesBatch(rows: TakeBatchInput[], opts?: BatchOpts): Promise; /** List takes filtered by holder/kind/active/etc. Resolves page_slug via JOIN. */ listTakes(opts?: TakesListOpts): Promise; diff --git a/src/core/pglite-engine.ts b/src/core/pglite-engine.ts index 5fb43ade1..cfb1e70c3 100644 --- a/src/core/pglite-engine.ts +++ b/src/core/pglite-engine.ts @@ -45,6 +45,8 @@ import type { import { validateSlug, contentHash, rowToPage, rowToStalePage, rowToChunk, rowToSearchResult, takeRowToTake, isUndefinedTableError, warnOncePerProcess } from './utils.ts'; import { deriveResolutionTuple, finalizeScorecard } from './takes-resolution.ts'; import { normalizeWeightForStorage } from './takes-fence.ts'; +import { executeRawJsonb } from './sql-query.ts'; +import { stripNul, buildLinkRows, buildTimelineRows, buildTakeRows } from './batch-rows.ts'; import { GBrainError, PAGE_SORT_SQL, ENRICH_ORDER_SQL } from './types.ts'; import { computeAnomaliesFromBuckets } from './cycle/anomaly.ts'; import { resolveBoostMap, resolveHardExcludes } from './search/source-boost.ts'; @@ -2426,7 +2428,7 @@ export class PGLiteEngine implements BrainEngine { ON CONFLICT (from_page_id, to_page_id, link_type, link_source, origin_page_id) DO UPDATE SET context = EXCLUDED.context, origin_field = EXCLUDED.origin_field`, - [from, to, linkType || '', context || '', src, originSlug ?? null, originField ?? null, fromSrc, toSrc, originSrc] + [from, to, linkType || '', stripNul(context || ''), src, originSlug ?? null, originField ?? null, fromSrc, toSrc, originSrc] ); } @@ -2437,39 +2439,31 @@ export class PGLiteEngine implements BrainEngine { private async _addLinksBatchOnce(links: LinkBatchInput[]): Promise { if (links.length === 0) return 0; - // unnest() pattern: 10 array-typed bound parameters regardless of batch - // size. Same shape as PostgresEngine (v0.18). Avoids the 65535-parameter - // cap. - // - // v0.18.0: every JOIN composite-keys on (slug, source_id) so the batch - // can't fan out across sources when the same slug exists in multiple - // sources. Origin JOIN uses LEFT JOIN on a composite key — NULL - // origin_slug leaves origin_page_id NULL, same as pre-v0.18. - const fromSlugs = links.map(l => l.from_slug); - const toSlugs = links.map(l => l.to_slug); - const linkTypes = links.map(l => l.link_type || ''); - const contexts = links.map(l => l.context || ''); - const linkSources = links.map(l => l.link_source || 'markdown'); - const originSlugs = links.map(l => l.origin_slug || null); - const originFields = links.map(l => l.origin_field || null); - const fromSourceIds = links.map(l => l.from_source_id || 'default'); - const toSourceIds = links.map(l => l.to_source_id || 'default'); - const originSourceIds = links.map(l => l.origin_source_id || 'default'); - // v0.41.18.0 (A10): link_kind column (v98). NULL = legacy/plain. - const linkKinds = links.map(l => l.link_kind ?? null); - const result = await this.db.query( + // #1861: JSONB jsonb_to_recordset instead of unnest(${arr}::text[]). The + // text[] array-literal path crashed Postgres on free-text context; JSONB + // encodes arbitrary text safely and dodges the 65535-param cap. Mirrors + // PostgresEngine exactly (engine parity); binding goes through the audited + // executeRawJsonb contract with an OBJECT wrapper { rows }. Composite + // (slug, source_id) JOINs + LEFT JOIN origin behavior are unchanged. + const rows = buildLinkRows(links); + const result = await executeRawJsonb( + this, `INSERT INTO links (from_page_id, to_page_id, link_type, context, link_source, link_kind, origin_page_id, origin_field) SELECT f.id, t.id, v.link_type, v.context, v.link_source, v.link_kind, o.id, v.origin_field - FROM unnest($1::text[], $2::text[], $3::text[], $4::text[], $5::text[], $6::text[], $7::text[], $8::text[], $9::text[], $10::text[], $11::text[]) - AS v(from_slug, to_slug, link_type, context, link_source, origin_slug, origin_field, from_source_id, to_source_id, origin_source_id, link_kind) + FROM jsonb_to_recordset(($1::jsonb)->'rows') AS v( + from_slug text, to_slug text, link_type text, context text, link_source text, + origin_slug text, origin_field text, from_source_id text, to_source_id text, + origin_source_id text, link_kind text + ) JOIN pages f ON f.slug = v.from_slug AND f.source_id = v.from_source_id JOIN pages t ON t.slug = v.to_slug AND t.source_id = v.to_source_id LEFT JOIN pages o ON o.slug = v.origin_slug AND o.source_id = v.origin_source_id ON CONFLICT (from_page_id, to_page_id, link_type, link_source, origin_page_id) DO NOTHING RETURNING 1`, - [fromSlugs, toSlugs, linkTypes, contexts, linkSources, originSlugs, originFields, fromSourceIds, toSourceIds, originSourceIds, linkKinds] + [], + [{ rows }], ); - return result.rows.length; + return result.length; } async removeLink( @@ -3103,23 +3097,23 @@ export class PGLiteEngine implements BrainEngine { private async _addTimelineEntriesBatchOnce(entries: TimelineBatchInput[]): Promise { if (entries.length === 0) return 0; - const slugs = entries.map(e => e.slug); - const dates = entries.map(e => e.date); - const sources = entries.map(e => e.source || ''); - const summaries = entries.map(e => e.summary); - const details = entries.map(e => e.detail || ''); - const sourceIds = entries.map(e => e.source_id || 'default'); - const result = await this.db.query( + // #1861: JSONB jsonb_to_recordset instead of unnest(${arr}::text[]); free-text + // summary/detail/source carry the same array-literal crash hazard. Mirrors + // PostgresEngine. `date` stays text in the recordset and is cast v.date::date. + const rows = buildTimelineRows(entries); + const result = await executeRawJsonb( + this, `INSERT INTO timeline_entries (page_id, date, source, summary, detail) SELECT p.id, v.date::date, v.source, v.summary, v.detail - FROM unnest($1::text[], $2::text[], $3::text[], $4::text[], $5::text[], $6::text[]) - AS v(slug, date, source, summary, detail, source_id) + FROM jsonb_to_recordset(($1::jsonb)->'rows') + AS v(slug text, date text, source text, summary text, detail text, source_id text) JOIN pages p ON p.slug = v.slug AND p.source_id = v.source_id ON CONFLICT (page_id, date, summary, source) DO NOTHING RETURNING 1`, - [slugs, dates, sources, summaries, details, sourceIds] + [], + [{ rows }], ); - return result.rows.length; + return result.length; } async getTimeline(slug: string, opts?: TimelineOpts): Promise { @@ -3794,34 +3788,33 @@ export class PGLiteEngine implements BrainEngine { // v0.28: Takes (typed/weighted/attributed claims) + synthesis_evidence // ============================================================ - async addTakesBatch(rowsIn: TakeBatchInput[]): Promise { + async addTakesBatch(rowsIn: TakeBatchInput[], opts?: BatchOpts): Promise { if (rowsIn.length === 0) return 0; - let weightClamped = 0; - const pageIds = rowsIn.map(r => r.page_id); - const rowNums = rowsIn.map(r => r.row_num); - const claims = rowsIn.map(r => r.claim); - const kinds = rowsIn.map(r => r.kind); - const holders = rowsIn.map(r => r.holder); - const weights = rowsIn.map(r => { - const { weight, clamped } = normalizeWeightForStorage(r.weight); - if (clamped) weightClamped++; - return weight; - }); - const sinces = rowsIn.map(r => r.since_date ?? null); - const untils = rowsIn.map(r => r.until_date ?? null); - const sources = rowsIn.map(r => r.source ?? null); - const supersededBys = rowsIn.map(r => r.superseded_by ?? null); - const actives = rowsIn.map(r => r.active ?? true); + // v0.42.26: wrap in batchRetry to match links/timeline (takes was the only + // batch primitive without retry resilience). + return this.batchRetry(opts?.auditSite ?? 'addTakesBatch', opts?.signal, () => this._addTakesBatchOnce(rowsIn), rowsIn.length); + } + + private async _addTakesBatchOnce(rowsIn: TakeBatchInput[]): Promise { + // #1861: JSONB jsonb_to_recordset instead of unnest(${arr}::text[]). Mirrors + // PostgresEngine: `claim` is free LLM prose (array-literal crash hazard); + // native recordset types + JSON-native numbers/booleans retire the per-engine + // element-type casts. Weight clamp/round + NUL strip live in buildTakeRows. + // ON CONFLICT DO UPDATE — intra-batch dup (page_id, row_num) errors, same as + // the pre-#1861 unnest path. + const { rows, weightClamped } = buildTakeRows(rowsIn); if (weightClamped > 0) { process.stderr.write(`[takes] TAKES_WEIGHT_CLAMPED: ${weightClamped} row(s) had weight outside [0,1]; clamped\n`); } - const result = await this.db.query( + const result = await executeRawJsonb( + this, `INSERT INTO takes (page_id, row_num, claim, kind, holder, weight, since_date, until_date, source, superseded_by, active) - SELECT v.page_id::int, v.row_num::int, v.claim, v.kind, v.holder, v.weight::real, - v.since_date::text, v.until_date::text, v.source, v.superseded_by::int, v.active::boolean - FROM unnest($1::int[], $2::int[], $3::text[], $4::text[], $5::text[], $6::real[], - $7::text[], $8::text[], $9::text[], $10::int[], $11::boolean[]) - AS v(page_id, row_num, claim, kind, holder, weight, since_date, until_date, source, superseded_by, active) + SELECT v.page_id, v.row_num, v.claim, v.kind, v.holder, v.weight, + v.since_date, v.until_date, v.source, v.superseded_by, v.active + FROM jsonb_to_recordset(($1::jsonb)->'rows') AS v( + page_id int, row_num int, claim text, kind text, holder text, weight real, + since_date text, until_date text, source text, superseded_by int, active boolean + ) ON CONFLICT (page_id, row_num) DO UPDATE SET claim = EXCLUDED.claim, kind = EXCLUDED.kind, @@ -3834,9 +3827,10 @@ export class PGLiteEngine implements BrainEngine { active = EXCLUDED.active, updated_at = now() RETURNING 1`, - [pageIds, rowNums, claims, kinds, holders, weights, sinces, untils, sources, supersededBys, actives] + [], + [{ rows }], ); - return result.rows.length; + return result.length; } /** v0.32.6 P1 — batched per-page active-takes fetch for the contradiction probe. */ diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index f1cc7a794..99ebfbf85 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -21,6 +21,8 @@ import type { import { MAX_SEARCH_LIMIT, clampSearchLimit } from './engine.ts'; import { deriveResolutionTuple, finalizeScorecard } from './takes-resolution.ts'; import { normalizeWeightForStorage } from './takes-fence.ts'; +import { executeRawJsonb } from './sql-query.ts'; +import { stripNul, buildLinkRows, buildTimelineRows, buildTakeRows } from './batch-rows.ts'; import { runMigrations } from './migrate.ts'; import { SCHEMA_SQL } from './schema-embedded.ts'; import { verifySchema } from './schema-verify.ts'; @@ -2509,7 +2511,7 @@ export class PostgresEngine implements BrainEngine { await sql` INSERT INTO links (from_page_id, to_page_id, link_type, context, link_source, origin_page_id, origin_field) SELECT f.id, t.id, v.link_type, v.context, v.link_source, o.id, v.origin_field - FROM (VALUES (${from}, ${to}, ${linkType || ''}, ${context || ''}, ${src}, ${originSlug ?? null}, ${originField ?? null}, ${fromSrc}, ${toSrc}, ${originSrc})) + FROM (VALUES (${from}, ${to}, ${linkType || ''}, ${stripNul(context || '')}, ${src}, ${originSlug ?? null}, ${originField ?? null}, ${fromSrc}, ${toSrc}, ${originSrc})) AS v(from_slug, to_slug, link_type, context, link_source, origin_slug, origin_field, from_source_id, to_source_id, origin_source_id) JOIN pages f ON f.slug = v.from_slug AND f.source_id = v.from_source_id JOIN pages t ON t.slug = v.to_slug AND t.source_id = v.to_source_id @@ -2526,42 +2528,33 @@ export class PostgresEngine implements BrainEngine { } private async _addLinksBatchOnce(links: LinkBatchInput[]): Promise { - const sql = this.sql; - // unnest() pattern: 7 array-typed bound parameters regardless of batch size. - // Avoids the 65535-parameter cap and the postgres-js sql(rows, ...) helper's - // identifier-escape gotcha when used inside a (VALUES) subquery. - // - // v0.13: added link_source, origin_slug, origin_field. Defaults: - // link_source → 'markdown' (back-compat with pre-v0.13 callers) - // origin_slug → NULL (resolves to origin_page_id IS NULL via LEFT JOIN) - // origin_field → NULL - const fromSlugs = links.map(l => l.from_slug); - const toSlugs = links.map(l => l.to_slug); - const linkTypes = links.map(l => l.link_type || ''); - const contexts = links.map(l => l.context || ''); - const linkSources = links.map(l => l.link_source || 'markdown'); - const originSlugs = links.map(l => l.origin_slug || null); - const originFields = links.map(l => l.origin_field || null); - const fromSourceIds = links.map(l => l.from_source_id || 'default'); - const toSourceIds = links.map(l => l.to_source_id || 'default'); - const originSourceIds = links.map(l => l.origin_source_id || 'default'); - // v0.41.18.0 (A10): link_kind column (v98). NULL = legacy/plain. - const linkKinds = links.map(l => l.link_kind ?? null); - const result = await sql` - INSERT INTO links (from_page_id, to_page_id, link_type, context, link_source, link_kind, origin_page_id, origin_field) - SELECT f.id, t.id, v.link_type, v.context, v.link_source, v.link_kind, o.id, v.origin_field - FROM unnest( - ${fromSlugs}::text[], ${toSlugs}::text[], ${linkTypes}::text[], - ${contexts}::text[], ${linkSources}::text[], ${originSlugs}::text[], - ${originFields}::text[], ${fromSourceIds}::text[], ${toSourceIds}::text[], - ${originSourceIds}::text[], ${linkKinds}::text[] - ) AS v(from_slug, to_slug, link_type, context, link_source, origin_slug, origin_field, from_source_id, to_source_id, origin_source_id, link_kind) - JOIN pages f ON f.slug = v.from_slug AND f.source_id = v.from_source_id - JOIN pages t ON t.slug = v.to_slug AND t.source_id = v.to_source_id - LEFT JOIN pages o ON o.slug = v.origin_slug AND o.source_id = v.origin_source_id - ON CONFLICT (from_page_id, to_page_id, link_type, link_source, origin_page_id) DO NOTHING - RETURNING 1 - `; + // #1861: pass the batch as one JSONB document via jsonb_to_recordset instead + // of N parallel unnest(${arr}::text[]). The old text[] array-literal path + // crashed Postgres ("malformed array literal") on free-text context strings + // (calendar/Zoom lines with commas, quotes, braces, em-dashes); JSONB encodes + // arbitrary text safely and dodges the 65535-param cap. Binding goes through + // executeRawJsonb (the audited cross-engine JSONB contract) with an OBJECT + // wrapper { rows } — a bare top-level array through postgres.js would re-enter + // the same array serializer this fix exists to avoid. Row construction + + // NUL-stripping + exact defaulting live in buildLinkRows (shared with PGLite). + const rows = buildLinkRows(links); + const result = await executeRawJsonb( + this, + `INSERT INTO links (from_page_id, to_page_id, link_type, context, link_source, link_kind, origin_page_id, origin_field) + SELECT f.id, t.id, v.link_type, v.context, v.link_source, v.link_kind, o.id, v.origin_field + FROM jsonb_to_recordset(($1::jsonb)->'rows') AS v( + from_slug text, to_slug text, link_type text, context text, link_source text, + origin_slug text, origin_field text, from_source_id text, to_source_id text, + origin_source_id text, link_kind text + ) + JOIN pages f ON f.slug = v.from_slug AND f.source_id = v.from_source_id + JOIN pages t ON t.slug = v.to_slug AND t.source_id = v.to_source_id + LEFT JOIN pages o ON o.slug = v.origin_slug AND o.source_id = v.origin_source_id + ON CONFLICT (from_page_id, to_page_id, link_type, link_source, origin_page_id) DO NOTHING + RETURNING 1`, + [], + [{ rows }], + ); return result.length; } @@ -3202,22 +3195,24 @@ export class PostgresEngine implements BrainEngine { } private async _addTimelineEntriesBatchOnce(entries: TimelineBatchInput[]): Promise { - const sql = this.sql; - const slugs = entries.map(e => e.slug); - const dates = entries.map(e => e.date); - const sources = entries.map(e => e.source || ''); - const summaries = entries.map(e => e.summary); - const details = entries.map(e => e.detail || ''); - const sourceIds = entries.map(e => e.source_id || 'default'); - const result = await sql` - INSERT INTO timeline_entries (page_id, date, source, summary, detail) - SELECT p.id, v.date::date, v.source, v.summary, v.detail - FROM unnest(${slugs}::text[], ${dates}::text[], ${sources}::text[], ${summaries}::text[], ${details}::text[], ${sourceIds}::text[]) - AS v(slug, date, source, summary, detail, source_id) - JOIN pages p ON p.slug = v.slug AND p.source_id = v.source_id - ON CONFLICT (page_id, date, summary, source) DO NOTHING - RETURNING 1 - `; + // #1861: JSONB jsonb_to_recordset instead of unnest(${arr}::text[]). Meeting + // summary/detail/source are free text with the same array-literal crash + // hazard as link context. See _addLinksBatchOnce for the full rationale. + // `date` stays text in the recordset and is cast v.date::date in the SELECT, + // exactly as the old unnest shape did. + const rows = buildTimelineRows(entries); + const result = await executeRawJsonb( + this, + `INSERT INTO timeline_entries (page_id, date, source, summary, detail) + SELECT p.id, v.date::date, v.source, v.summary, v.detail + FROM jsonb_to_recordset(($1::jsonb)->'rows') + AS v(slug text, date text, source text, summary text, detail text, source_id text) + JOIN pages p ON p.slug = v.slug AND p.source_id = v.source_id + ON CONFLICT (page_id, date, summary, source) DO NOTHING + RETURNING 1`, + [], + [{ rows }], + ); return result.length; } @@ -3883,53 +3878,51 @@ export class PostgresEngine implements BrainEngine { // v0.28: Takes (typed/weighted/attributed claims) + synthesis_evidence // ============================================================ - async addTakesBatch(rowsIn: TakeBatchInput[]): Promise { + async addTakesBatch(rowsIn: TakeBatchInput[], opts?: BatchOpts): Promise { if (rowsIn.length === 0) return 0; - const sql = this.sql; - let weightClamped = 0; - const pageIds = rowsIn.map(r => r.page_id); - const rowNums = rowsIn.map(r => r.row_num); - const claims = rowsIn.map(r => r.claim); - const kinds = rowsIn.map(r => r.kind); - const holders = rowsIn.map(r => r.holder); - const weights = rowsIn.map(r => { - const { weight, clamped } = normalizeWeightForStorage(r.weight); - if (clamped) weightClamped++; - return weight; - }); - const sinces = rowsIn.map(r => r.since_date ?? null); - const untils = rowsIn.map(r => r.until_date ?? null); - const sources = rowsIn.map(r => r.source ?? null); - const supersededBys = rowsIn.map(r => r.superseded_by ?? null); - // postgres-js needs boolean arrays passed as text[] then SQL-cast to boolean[], - // otherwise the driver mis-detects element type. Same pattern as how the - // existing batch methods handle bools. - const actives = rowsIn.map(r => (r.active ?? true) ? 'true' : 'false'); + // v0.42.26: takes is a batch primitive too — wrap in batchRetry so a + // Supavisor circuit-breaker blip doesn't silently drop takes the way it + // could before (links/timeline already had this; takes was the gap). + return this.batchRetry(opts?.auditSite ?? 'addTakesBatch', opts?.signal, () => this._addTakesBatchOnce(rowsIn), rowsIn.length); + } + + private async _addTakesBatchOnce(rowsIn: TakeBatchInput[]): Promise { + // #1861: JSONB jsonb_to_recordset instead of unnest(${arr}::text[]). `claim` + // is free LLM-extracted prose with the same array-literal crash hazard as + // link context. JSONB additionally lets us declare NATIVE recordset column + // types and emit JSON-native numbers/booleans, which retires the old + // postgres-js ${actives}::text[]::boolean[] element-type workaround entirely. + // Weight clamp/round + NUL-stripping live in buildTakeRows (shared w/ PGLite). + // NOTE: ON CONFLICT here is DO UPDATE (not DO NOTHING) — an intra-batch + // duplicate (page_id, row_num) errors, identical to the pre-#1861 unnest path. + const { rows, weightClamped } = buildTakeRows(rowsIn); if (weightClamped > 0) { process.stderr.write(`[takes] TAKES_WEIGHT_CLAMPED: ${weightClamped} row(s) had weight outside [0,1]; clamped\n`); } - const result = await sql` - INSERT INTO takes (page_id, row_num, claim, kind, holder, weight, since_date, until_date, source, superseded_by, active) - SELECT v.page_id::int, v.row_num::int, v.claim, v.kind, v.holder, v.weight::real, - v.since_date::text, v.until_date::text, v.source, v.superseded_by::int, v.active::boolean - FROM unnest( - ${pageIds}::int[], ${rowNums}::int[], ${claims}::text[], ${kinds}::text[], - ${holders}::text[], ${weights}::real[], ${sinces}::text[], ${untils}::text[], - ${sources}::text[], ${supersededBys}::int[], ${actives}::text[]::boolean[] - ) AS v(page_id, row_num, claim, kind, holder, weight, since_date, until_date, source, superseded_by, active) - ON CONFLICT (page_id, row_num) DO UPDATE SET - claim = EXCLUDED.claim, - kind = EXCLUDED.kind, - holder = EXCLUDED.holder, - weight = EXCLUDED.weight, - since_date = EXCLUDED.since_date, - until_date = EXCLUDED.until_date, - source = EXCLUDED.source, - superseded_by = EXCLUDED.superseded_by, - active = EXCLUDED.active, - updated_at = now() - RETURNING 1 - `; + const result = await executeRawJsonb( + this, + `INSERT INTO takes (page_id, row_num, claim, kind, holder, weight, since_date, until_date, source, superseded_by, active) + SELECT v.page_id, v.row_num, v.claim, v.kind, v.holder, v.weight, + v.since_date, v.until_date, v.source, v.superseded_by, v.active + FROM jsonb_to_recordset(($1::jsonb)->'rows') AS v( + page_id int, row_num int, claim text, kind text, holder text, weight real, + since_date text, until_date text, source text, superseded_by int, active boolean + ) + ON CONFLICT (page_id, row_num) DO UPDATE SET + claim = EXCLUDED.claim, + kind = EXCLUDED.kind, + holder = EXCLUDED.holder, + weight = EXCLUDED.weight, + since_date = EXCLUDED.since_date, + until_date = EXCLUDED.until_date, + source = EXCLUDED.source, + superseded_by = EXCLUDED.superseded_by, + active = EXCLUDED.active, + updated_at = now() + RETURNING 1`, + [], + [{ rows }], + ); return result.length; } diff --git a/src/core/retry.ts b/src/core/retry.ts index a5a36ef06..5defca720 100644 --- a/src/core/retry.ts +++ b/src/core/retry.ts @@ -76,6 +76,7 @@ export const BATCH_AUDIT_SITES = [ // Engine-method defaults (used when caller doesn't supply auditSite). 'addLinksBatch', 'addTimelineEntriesBatch', + 'addTakesBatch', 'upsertChunks', // extract.ts per-site labels. 'extract.links_inc', diff --git a/src/core/sql-query.ts b/src/core/sql-query.ts index 6483af8f1..2fcae4556 100644 --- a/src/core/sql-query.ts +++ b/src/core/sql-query.ts @@ -113,9 +113,24 @@ export async function executeRawJsonb>( for (const value of scalarParams) { assertSqlValue(value); } - // jsonbParams are explicitly NOT validated as scalar — they're meant to - // hold JS objects/arrays that postgres.js / PGLite will encode as JSONB - // via the explicit ::jsonb cast in the caller's SQL string. + // jsonbParams hold JS objects (or null) that postgres.js / PGLite encode as + // JSONB via the explicit `::jsonb` cast in the caller's SQL string. A + // top-level ARRAY is rejected: postgres.js can bind a bare JS array as a + // Postgres ARRAY literal rather than jsonb, which silently re-enters the + // "malformed array literal" class gbrain#1861 exists to escape. Wrap arrays + // in an object, e.g. `[{ rows: [...] }]` selected via + // `jsonb_to_recordset(($N::jsonb)->'rows')`. This enforces at the call layer + // the invariant the batch-insert methods rely on (codex #1861 P2a). + for (const value of jsonbParams) { + if (Array.isArray(value)) { + throw new TypeError( + 'executeRawJsonb: a top-level array jsonb param can bind as a Postgres ' + + 'array literal (not jsonb) through postgres.js. Wrap it in an object — ' + + "e.g. `[{ rows: [...] }]` with `jsonb_to_recordset(($N::jsonb)->'rows')`. " + + '(gbrain#1861)', + ); + } + } const params: unknown[] = [...scalarParams, ...jsonbParams]; return engine.executeRaw(sql, params); } diff --git a/test/core/retry.test.ts b/test/core/retry.test.ts index 6bd214709..77746da9d 100644 --- a/test/core/retry.test.ts +++ b/test/core/retry.test.ts @@ -339,7 +339,7 @@ describe('BATCH_AUDIT_SITES typed enum + isBatchAuditSite guard (D10c codex)', ( // Pin the set so a future "cleanup" PR can't silently drop a site and // break audit-attribution for the corresponding caller. const expected = new Set([ - 'addLinksBatch', 'addTimelineEntriesBatch', 'upsertChunks', + 'addLinksBatch', 'addTimelineEntriesBatch', 'addTakesBatch', 'upsertChunks', 'extract.links_inc', 'extract.timeline_inc', 'extract.links_fs', 'extract.timeline_fs', 'extract.links_db', 'extract.timeline_db', diff --git a/test/e2e/jsonb-batch-poison-postgres.test.ts b/test/e2e/jsonb-batch-poison-postgres.test.ts new file mode 100644 index 000000000..ca49f9fcd --- /dev/null +++ b/test/e2e/jsonb-batch-poison-postgres.test.ts @@ -0,0 +1,103 @@ +// gbrain#1861 regression — Postgres lane (DATABASE_URL-gated). +// +// This is the engine that actually crashed: postgres.js serialized free-text +// `context` into a Postgres text[] literal that `array_in` rejected with +// "malformed array literal", aborting the whole `extract links --stale` sweep. +// The PGLite sibling (test/links-timeline-jsonb-poison.test.ts) may not +// reproduce the original crash because PGLite uses a different array serializer, +// so this gated test is the true regression lock. It runs in CI lanes that set +// DATABASE_URL and skips gracefully elsewhere. + +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'bun:test'; +import { hasDatabase, setupDB, teardownDB, getEngine, getConn } from './helpers.ts'; +import type { PostgresEngine } from '../../src/core/postgres-engine.ts'; + +const SKIP = !hasDatabase(); +const d = SKIP ? describe.skip : describe; + +const POISON = + 'Zoom: https://zoom.us/j/95178948505?pwd=YmdFRWxXbWZadlNkaG9iNC9CYW12QT09, ' + + '"Q2 sync" — notes {a,b}, path C:\\Users\\x, em–dash } and { trailing'; +const NUL = String.fromCharCode(0); + +let engine: PostgresEngine; + +async function seed(slug: string) { + await engine.putPage(slug, { + title: slug, type: 'concept' as never, + compiled_truth: 'body long enough to pass any minimum length backstop', + timeline: '', frontmatter: {}, source_path: `${slug}.md`, + }); +} + +async function pageId(slug: string): Promise { + const rows = await engine.executeRaw<{ id: number }>( + `SELECT id FROM pages WHERE slug = $1 AND source_id = 'default'`, [slug], + ); + return rows[0].id; +} + +d('JSONB batch poison — Postgres (#1861)', () => { + beforeAll(async () => { engine = await setupDB(); }); + afterAll(async () => { await teardownDB(); }); + beforeEach(async () => { + // Clean the three target tables between cases. + const conn = getConn(); + await conn.unsafe('TRUNCATE links, timeline_entries, takes, pages CASCADE'); + }); + + it('addLinksBatch round-trips poison context (the original crash)', async () => { + await seed('from-page'); await seed('to-page'); + const n = await engine.addLinksBatch([ + { from_slug: 'from-page', to_slug: 'to-page', link_type: 'mentions', + context: POISON, link_source: 'manual' }, + ]); + expect(n).toBe(1); + const links = await engine.getLinks('from-page', { sourceId: 'default' }); + expect(links[0].context).toBe(POISON); + }); + + it('strips embedded NUL in link context', async () => { + await seed('a'); await seed('b'); + await engine.addLinksBatch([ + { from_slug: 'a', to_slug: 'b', link_type: 'mentions', + context: `before${NUL}after`, link_source: 'manual' }, + ]); + const links = await engine.getLinks('a', { sourceId: 'default' }); + expect(links[0].context).toBe('beforeafter'); + }); + + it('addTimelineEntriesBatch round-trips poison summary/detail/source', async () => { + await seed('meeting-page'); + const n = await engine.addTimelineEntriesBatch([ + { slug: 'meeting-page', date: '2026-06-04', source: POISON, + summary: POISON, detail: POISON }, + ]); + expect(n).toBe(1); + const rows = await engine.executeRaw<{ summary: string; detail: string; source: string }>( + `SELECT te.summary, te.detail, te.source FROM timeline_entries te + JOIN pages p ON p.id = te.page_id WHERE p.slug = 'meeting-page'`, + ); + expect(rows[0].summary).toBe(POISON); + expect(rows[0].detail).toBe(POISON); + expect(rows[0].source).toBe(POISON); + }); + + it('addTakesBatch round-trips poison claim + native number/bool/null', async () => { + await seed('take-page'); + const pid = await pageId('take-page'); + const n = await engine.addTakesBatch([ + { page_id: pid, row_num: 1, claim: POISON, kind: 'fact', holder: 'garry', + weight: 0.74, active: false }, + ]); + expect(n).toBe(1); + const rows = await engine.executeRaw<{ + claim: string; weight: number | string; active: boolean; since_date: string | null; + }>(`SELECT claim, weight, active, since_date FROM takes t + JOIN pages p ON p.id = t.page_id WHERE p.slug = 'take-page' AND t.row_num = 1`); + expect(rows[0].claim).toBe(POISON); + expect(Number(rows[0].weight)).toBeCloseTo(0.75, 5); + expect(rows[0].active).toBe(false); + expect(rows[0].since_date).toBeNull(); + }); +}); diff --git a/test/helpers/no-anthropic-key.ts b/test/helpers/no-anthropic-key.ts new file mode 100644 index 000000000..ddf4f77c3 --- /dev/null +++ b/test/helpers/no-anthropic-key.ts @@ -0,0 +1,41 @@ +/** + * Run `fn` with NO Anthropic key reachable from EITHER source the gateway + * checks. `hasAnthropicKey()` (src/core/ai/anthropic-key.ts) returns true if + * `process.env.ANTHROPIC_API_KEY` is set OR `~/.gbrain/config.json` carries + * `anthropic_api_key`. A test that only `delete`s the env var is NOT hermetic: + * on a developer machine whose `~/.gbrain` holds a real key (or whose + * `.env.testing` sets ANTHROPIC_API_KEY), the "no key" path actually fires a + * live LLM call and the assertion flips from `NO_ANTHROPIC_API_KEY` to + * `LLM_OUTPUT_NOT_JSON`. + * + * This helper neutralizes BOTH sources for the duration of `fn`: + * - deletes ANTHROPIC_API_KEY from the env, and + * - points GBRAIN_HOME at a fresh empty temp dir so `configDir()` (which + * honors GBRAIN_HOME) resolves to a directory with no config.json, making + * `loadConfig()` return null. + * + * Both are restored (and the temp dir removed) in a finally, even on throw. + * `loadConfig()` reads the file fresh on every call and `probeChatModel` runs + * before the gateway's model cache, so per-call isolation is sufficient — no + * module-level key state survives. + */ +import { mkdtempSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +export async function withoutAnthropicKey(fn: () => Promise): Promise { + const origKey = process.env.ANTHROPIC_API_KEY; + const origHome = process.env.GBRAIN_HOME; + const tmp = mkdtempSync(join(tmpdir(), 'gbrain-nokey-')); + delete process.env.ANTHROPIC_API_KEY; + process.env.GBRAIN_HOME = tmp; // configDir() -> $GBRAIN_HOME/.gbrain (absent -> no config key) + try { + return await fn(); + } finally { + if (origKey !== undefined) process.env.ANTHROPIC_API_KEY = origKey; + else delete process.env.ANTHROPIC_API_KEY; + if (origHome !== undefined) process.env.GBRAIN_HOME = origHome; + else delete process.env.GBRAIN_HOME; + try { rmSync(tmp, { recursive: true, force: true }); } catch { /* best-effort */ } + } +} diff --git a/test/links-timeline-jsonb-poison.test.ts b/test/links-timeline-jsonb-poison.test.ts new file mode 100644 index 000000000..4d6a7cd97 --- /dev/null +++ b/test/links-timeline-jsonb-poison.test.ts @@ -0,0 +1,246 @@ +// gbrain#1861 regression — batch inserts must survive free-text "poison" +// payloads (calendar/Zoom context: commas, quotes, backslashes, braces, +// em-dashes) that the old unnest(${arr}::text[]) array-literal path rejected +// with Postgres "malformed array literal". +// +// PGLite half (always-on). PGLite uses a different array serializer than +// postgres.js, so it may not reproduce the ORIGINAL crash — but it locks the +// new jsonb_to_recordset path's behavior everywhere CI runs. The Postgres lane +// (test/e2e/jsonb-batch-poison-postgres.test.ts) is the one that actually +// crashed pre-fix. + +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; + +// The exact shape from the #1861 crash report: Zoom URL with ?pwd=, commas, +// double-quotes, a Windows backslash path, braces, and an em-dash. +const POISON = + 'Zoom: https://zoom.us/j/95178948505?pwd=YmdFRWxXbWZadlNkaG9iNC9CYW12QT09, ' + + '"Q2 sync" — notes {a,b}, path C:\\Users\\x, em–dash } and { trailing'; + +// U+0000 — Postgres jsonb rejects it; the row builders strip it. Built via +// fromCharCode so no literal NUL byte ever lands in this source file. +const NUL = String.fromCharCode(0); + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); + +async function seed(slug: string) { + await engine.putPage(slug, { + title: slug, type: 'concept' as never, + compiled_truth: 'body long enough to pass any minimum length backstop', + timeline: '', frontmatter: {}, source_path: `${slug}.md`, + }); +} + +async function pageId(slug: string): Promise { + const rows = await engine.executeRaw<{ id: number }>( + `SELECT id FROM pages WHERE slug = $1 AND source_id = 'default'`, [slug], + ); + return rows[0].id; +} + +describe('addLinksBatch — JSONB poison (#1861)', () => { + it('round-trips calendar free-text context without throwing', async () => { + await seed('from-page'); + await seed('to-page'); + const n = await engine.addLinksBatch([ + { from_slug: 'from-page', to_slug: 'to-page', link_type: 'mentions', + context: POISON, link_source: 'manual' }, + ]); + expect(n).toBe(1); + const links = await engine.getLinks('from-page', { sourceId: 'default' }); + expect(links).toHaveLength(1); + expect(links[0].context).toBe(POISON); // byte-identical + }); + + it('strips embedded NUL but preserves the rest', async () => { + await seed('a'); await seed('b'); + await engine.addLinksBatch([ + { from_slug: 'a', to_slug: 'b', link_type: 'mentions', + context: `before${NUL}after`, link_source: 'manual' }, + ]); + const links = await engine.getLinks('a', { sourceId: 'default' }); + expect(links[0].context).toBe('beforeafter'); + }); + + it('null origin_slug leaves origin_page_id NULL', async () => { + await seed('a'); await seed('b'); + await engine.addLinksBatch([ + { from_slug: 'a', to_slug: 'b', link_type: 'mentions', link_source: 'manual' }, + ]); + const rows = await engine.executeRaw<{ origin_page_id: number | null }>( + `SELECT l.origin_page_id FROM links l JOIN pages p ON p.id = l.from_page_id + WHERE p.slug = 'a'`, + ); + expect(rows[0].origin_page_id).toBeNull(); + }); + + it('collapses an intra-batch duplicate (ON CONFLICT DO NOTHING)', async () => { + await seed('a'); await seed('b'); + const dup = { from_slug: 'a', to_slug: 'b', link_type: 'mentions', + context: POISON, link_source: 'manual' as const }; + const n = await engine.addLinksBatch([dup, dup]); + expect(n).toBe(1); + const links = await engine.getLinks('a', { sourceId: 'default' }); + expect(links).toHaveLength(1); + }); + + it('round-trips a non-null link_kind through the jsonb recordset', async () => { + await seed('a'); await seed('b'); + await engine.addLinksBatch([ + { from_slug: 'a', to_slug: 'b', link_type: 'mentions', context: POISON, + link_source: 'mentions', link_kind: 'typed_ner' }, + ]); + const rows = await engine.executeRaw<{ link_kind: string | null }>( + `SELECT l.link_kind FROM links l JOIN pages p ON p.id = l.from_page_id + WHERE p.slug = 'a'`, + ); + expect(rows[0].link_kind).toBe('typed_ner'); // new recordset column, exercised non-NULL + }); + + it('leaves link_kind NULL when omitted (legacy/plain)', async () => { + await seed('a'); await seed('b'); + await engine.addLinksBatch([ + { from_slug: 'a', to_slug: 'b', link_type: 'mentions', link_source: 'manual' }, + ]); + const rows = await engine.executeRaw<{ link_kind: string | null }>( + `SELECT l.link_kind FROM links l JOIN pages p ON p.id = l.from_page_id + WHERE p.slug = 'a'`, + ); + expect(rows[0].link_kind).toBeNull(); + }); +}); + +describe('addTimelineEntriesBatch — JSONB poison (#1861)', () => { + it('round-trips free-text summary/detail/source without throwing', async () => { + await seed('meeting-page'); + const n = await engine.addTimelineEntriesBatch([ + { slug: 'meeting-page', date: '2026-06-04', source: POISON, + summary: POISON, detail: POISON }, + ]); + expect(n).toBe(1); + const rows = await engine.executeRaw<{ summary: string; detail: string; source: string }>( + `SELECT te.summary, te.detail, te.source FROM timeline_entries te + JOIN pages p ON p.id = te.page_id WHERE p.slug = 'meeting-page'`, + ); + expect(rows[0].summary).toBe(POISON); + expect(rows[0].detail).toBe(POISON); + expect(rows[0].source).toBe(POISON); + }); + + it('strips embedded NUL in summary', async () => { + await seed('m'); + await engine.addTimelineEntriesBatch([ + { slug: 'm', date: '2026-06-04', summary: `a${NUL}b`, detail: '' }, + ]); + const rows = await engine.executeRaw<{ summary: string }>( + `SELECT te.summary FROM timeline_entries te JOIN pages p ON p.id = te.page_id + WHERE p.slug = 'm'`, + ); + expect(rows[0].summary).toBe('ab'); + }); +}); + +describe('addTakesBatch — JSONB poison + native-type parity (#1861)', () => { + it('round-trips free-text claim and native number/bool/null fields', async () => { + await seed('take-page'); + const pid = await pageId('take-page'); + const n = await engine.addTakesBatch([ + { page_id: pid, row_num: 1, claim: POISON, kind: 'fact', holder: 'garry', + weight: 0.74, active: false }, // since_date/until_date/source omitted -> null + ]); + expect(n).toBe(1); + const rows = await engine.executeRaw<{ + claim: string; weight: number | string; active: boolean; since_date: string | null; + }>(`SELECT claim, weight, active, since_date FROM takes t + JOIN pages p ON p.id = t.page_id WHERE p.slug = 'take-page' AND t.row_num = 1`); + expect(rows[0].claim).toBe(POISON); // free-text claim survives + expect(Number(rows[0].weight)).toBeCloseTo(0.75, 5); // 0.74 -> 0.05 grid + expect(rows[0].active).toBe(false); // JSON boolean round-trips + expect(rows[0].since_date).toBeNull(); // omitted -> SQL NULL + }); + + it('strips embedded NUL from the free-text claim', async () => { + await seed('tp-nul'); + const pid = await pageId('tp-nul'); + await engine.addTakesBatch([ + { page_id: pid, row_num: 1, claim: `a${NUL}b`, kind: 'fact', holder: 'h' }, + ]); + const rows = await engine.executeRaw<{ claim: string }>( + `SELECT claim FROM takes t JOIN pages p ON p.id = t.page_id + WHERE p.slug = 'tp-nul' AND t.row_num = 1`, + ); + expect(rows[0].claim).toBe('ab'); + }); + + it('clamps out-of-range weight (>1) to 1.0', async () => { + await seed('tp2'); + const pid = await pageId('tp2'); + await engine.addTakesBatch([ + { page_id: pid, row_num: 1, claim: 'c', kind: 'fact', holder: 'h', weight: 1.5 }, + ]); + const rows = await engine.executeRaw<{ weight: number | string }>( + `SELECT weight FROM takes t JOIN pages p ON p.id = t.page_id + WHERE p.slug = 'tp2' AND t.row_num = 1`, + ); + expect(Number(rows[0].weight)).toBeCloseTo(1.0, 5); + }); + + it('DO UPDATE upserts an existing (page_id, row_num)', async () => { + await seed('tp3'); + const pid = await pageId('tp3'); + await engine.addTakesBatch([ + { page_id: pid, row_num: 1, claim: 'first', kind: 'fact', holder: 'h' }, + ]); + await engine.addTakesBatch([ + { page_id: pid, row_num: 1, claim: 'second', kind: 'fact', holder: 'h' }, + ]); + const rows = await engine.executeRaw<{ claim: string; n: number | string }>( + `SELECT claim, (SELECT count(*) FROM takes t2 JOIN pages p2 ON p2.id = t2.page_id + WHERE p2.slug = 'tp3') AS n + FROM takes t JOIN pages p ON p.id = t.page_id WHERE p.slug = 'tp3' AND t.row_num = 1`, + ); + expect(rows[0].claim).toBe('second'); + expect(Number(rows[0].n)).toBe(1); + }); + + it('round-trips nullable/native take fields (superseded_by, until_date, non-null source)', async () => { + await seed('tp4'); + const pid = await pageId('tp4'); + await engine.addTakesBatch([ + { page_id: pid, row_num: 1, claim: 'c', kind: 'fact', holder: 'h', + superseded_by: 7, until_date: '2027-01-01', source: POISON }, + ]); + const rows = await engine.executeRaw<{ + superseded_by: number | string | null; until_date: string | null; source: string; + }>(`SELECT superseded_by, until_date::text AS until_date, source FROM takes t + JOIN pages p ON p.id = t.page_id WHERE p.slug = 'tp4' AND t.row_num = 1`); + expect(Number(rows[0].superseded_by)).toBe(7); // native int column + expect(rows[0].until_date).toBe('2027-01-01'); // text/date round-trip + expect(rows[0].source).toBe(POISON); // non-null free-text source + }); +}); + +describe('scalar addLink — NUL strip on free-text context (#1861 codex #3)', () => { + it('addLink strips NUL from context', async () => { + await seed('a'); await seed('b'); + await engine.addLink('a', 'b', `before${NUL}after`, 'mentions', 'manual'); + const links = await engine.getLinks('a', { sourceId: 'default' }); + expect(links[0].context).toBe('beforeafter'); + }); +}); diff --git a/test/sql-query.test.ts b/test/sql-query.test.ts index c9f57a515..ce6d7f9c0 100644 --- a/test/sql-query.test.ts +++ b/test/sql-query.test.ts @@ -172,4 +172,20 @@ describe('executeRawJsonb (D1 wave / v0.31)', () => { ), ).rejects.toThrow(/only supports scalar bind values/); }); + + test('rejects a top-level array jsonb param (gbrain#1861 P2a guard)', async () => { + // A bare JS array bound to a $N::jsonb position can serialize as a Postgres + // array literal (not jsonb) through postgres.js, re-entering the + // "malformed array literal" class #1861 escaped. The helper must reject it + // and steer callers to the { rows: [...] } wrapper. Objects + null are fine + // (covered by the tests above). + await expect( + executeRawJsonb( + engine, + `INSERT INTO nope (j) VALUES ($1::jsonb)`, + [], + [[{ a: 1 }, { a: 2 }] as any], + ), + ).rejects.toThrow(/top-level array jsonb param/); + }); }); diff --git a/test/takes-mcp-allowlist.serial.test.ts b/test/takes-mcp-allowlist.serial.test.ts index d168a376a..3720bf4a8 100644 --- a/test/takes-mcp-allowlist.serial.test.ts +++ b/test/takes-mcp-allowlist.serial.test.ts @@ -13,6 +13,7 @@ * This test exercises step 3-5 directly through dispatchToolCall. */ import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { withoutAnthropicKey } from './helpers/no-anthropic-key.ts'; import { PGLiteEngine } from '../src/core/pglite-engine.ts'; import { dispatchToolCall } from '../src/mcp/dispatch.ts'; import { TAKES_FENCE_BEGIN, TAKES_FENCE_END } from '../src/core/takes-fence.ts'; @@ -205,45 +206,35 @@ describe('per-token takes-holder allow-list — get_versions body channel', () = describe('think op — read-only on remote callers (Lane D landed)', () => { test('remote save/take is forced read-only via remote_persisted_blocked flag', async () => { - // Without ANTHROPIC_API_KEY, runThink returns gather-only result with NO_ANTHROPIC_API_KEY warning. - const origKey = process.env.ANTHROPIC_API_KEY; - delete process.env.ANTHROPIC_API_KEY; - try { - const result = await dispatchToolCall(engine, 'think', { question: 'q', save: true, take: true }, { - remote: true, - takesHoldersAllowList: ['world', 'garry', 'brain'], - }); - const env = parseResult(result) as { - remote_persisted_blocked: boolean; - saved_slug: string | null; - warnings: string[]; - }; - // Codex P1 #7: remote save/take is silently disabled. - expect(env.remote_persisted_blocked).toBe(true); - expect(env.saved_slug).toBeNull(); - // Without API key, gather succeeds but synthesis is skipped. - expect(env.warnings).toContain('NO_ANTHROPIC_API_KEY'); - } finally { - if (origKey) process.env.ANTHROPIC_API_KEY = origKey; - } + // Hermetic no-key: neutralize BOTH env var AND ~/.gbrain config key, else a + // configured machine fires a real LLM call and the warning flips to + // LLM_OUTPUT_NOT_JSON. runThink then returns gather-only + NO_ANTHROPIC_API_KEY. + const result = await withoutAnthropicKey(() => dispatchToolCall(engine, 'think', { question: 'q', save: true, take: true }, { + remote: true, + takesHoldersAllowList: ['world', 'garry', 'brain'], + })); + const env = parseResult(result) as { + remote_persisted_blocked: boolean; + saved_slug: string | null; + warnings: string[]; + }; + // Codex P1 #7: remote save/take is silently disabled. + expect(env.remote_persisted_blocked).toBe(true); + expect(env.saved_slug).toBeNull(); + // Without API key, gather succeeds but synthesis is skipped. + expect(env.warnings).toContain('NO_ANTHROPIC_API_KEY'); }); test('local-CLI think runs full pipeline (gather-only without API key)', async () => { - const origKey = process.env.ANTHROPIC_API_KEY; - delete process.env.ANTHROPIC_API_KEY; - try { - const result = await dispatchToolCall(engine, 'think', { question: 'q', save: true }, { - remote: false, - }); - const env = parseResult(result) as { - warnings: string[]; - remote_persisted_blocked: boolean; - }; - expect(env.remote_persisted_blocked).toBe(false); - // Without API key, returns gather-only + warning. With key, would actually synthesize. - expect(env.warnings).toContain('NO_ANTHROPIC_API_KEY'); - } finally { - if (origKey) process.env.ANTHROPIC_API_KEY = origKey; - } + const result = await withoutAnthropicKey(() => dispatchToolCall(engine, 'think', { question: 'q', save: true }, { + remote: false, + })); + const env = parseResult(result) as { + warnings: string[]; + remote_persisted_blocked: boolean; + }; + expect(env.remote_persisted_blocked).toBe(false); + // Without API key, returns gather-only + warning. With key, would actually synthesize. + expect(env.warnings).toContain('NO_ANTHROPIC_API_KEY'); }); }); diff --git a/test/think-pipeline.serial.test.ts b/test/think-pipeline.serial.test.ts index cf8be936b..d2e1a5442 100644 --- a/test/think-pipeline.serial.test.ts +++ b/test/think-pipeline.serial.test.ts @@ -5,6 +5,7 @@ import { runThink, persistSynthesis, type ThinkLLMClient } from '../src/core/thi import { sanitizeTakeForPrompt, renderTakesBlock } from '../src/core/think/sanitize.ts'; import { resolveCitations, parseInlineCitations, normalizeStructuredCitations } from '../src/core/think/cite-render.ts'; import { runGather } from '../src/core/think/gather.ts'; +import { withoutAnthropicKey } from './helpers/no-anthropic-key.ts'; let engine: PGLiteEngine; let alicePageId: number; @@ -207,16 +208,13 @@ describe('runThink (with stub client)', () => { }); test('degrades gracefully without ANTHROPIC_API_KEY', async () => { - const origKey = process.env.ANTHROPIC_API_KEY; - delete process.env.ANTHROPIC_API_KEY; - try { - const result = await runThink(engine, { question: 'no key test' }); - expect(result.warnings).toContain('NO_ANTHROPIC_API_KEY'); - expect(result.answer).toContain('no LLM available'); - expect(result.rounds).toBe(0); - } finally { - if (origKey) process.env.ANTHROPIC_API_KEY = origKey; - } + // Hermetic: neutralize BOTH the env var AND ~/.gbrain config key, else a + // developer/CI machine with a configured key fires a real LLM call and this + // assertion flips to LLM_OUTPUT_NOT_JSON. + const result = await withoutAnthropicKey(() => runThink(engine, { question: 'no key test' })); + expect(result.warnings).toContain('NO_ANTHROPIC_API_KEY'); + expect(result.answer).toContain('no LLM available'); + expect(result.rounds).toBe(0); }); test('persistSynthesis writes synthesis page + evidence rows', async () => { @@ -285,16 +283,11 @@ describe('runThink — #1698 explicit-model hard error', () => { }); test('NON-explicit bad model does NOT throw — graceful degrade (no modelExplicit)', async () => { - const origKey = process.env.ANTHROPIC_API_KEY; - delete process.env.ANTHROPIC_API_KEY; - try { - // model present but modelExplicit unset → early gate skipped; builder returns null. - const result = await runThink(engine, { question: 'nonexplicit bad', model: 'bogusprovider:foo' }); - expect(result.warnings).toContain('NO_ANTHROPIC_API_KEY'); - expect(result.synthesisOk).toBe(false); - } finally { - if (origKey) process.env.ANTHROPIC_API_KEY = origKey; - } + // model present but modelExplicit unset → early gate skipped; builder returns null. + // Hermetic no-key so the assertion can't be perturbed by a configured key. + const result = await withoutAnthropicKey(() => runThink(engine, { question: 'nonexplicit bad', model: 'bogusprovider:foo' })); + expect(result.warnings).toContain('NO_ANTHROPIC_API_KEY'); + expect(result.synthesisOk).toBe(false); }); }); @@ -373,15 +366,11 @@ describe('think MCP op — #1698 C3 + #10', () => { test('#10: local save with no synthesis → saved_slug is null, not "" + warning surfaced', async () => { const op = operationsByName['think']; - const origKey = process.env.ANTHROPIC_API_KEY; - delete process.env.ANTHROPIC_API_KEY; - try { - // Local (remote:false), save:true, no key → graceful stub → persist-skip. - const res: any = await op.handler(baseCtx(false) as any, { question: 'op empty save test', save: true }); - expect(res.saved_slug).toBeNull(); - expect(res.warnings).toContain('SYNTHESIS_EMPTY_NOT_PERSISTED'); - } finally { - if (origKey) process.env.ANTHROPIC_API_KEY = origKey; - } + // Hermetic no-key: synthesisOk=false → persistSynthesis returns + // SYNTHESIS_EMPTY_NOT_PERSISTED deterministically (was previously at the + // mercy of whatever a live LLM returned for this prompt). + const res: any = await withoutAnthropicKey(() => op.handler(baseCtx(false) as any, { question: 'op empty save test', save: true })); + expect(res.saved_slug).toBeNull(); + expect(res.warnings).toContain('SYNTHESIS_EMPTY_NOT_PERSISTED'); }); });