From d8a1c893f0281ae1cb3259323b7e493816233da2 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 02:52:05 +0300 Subject: [PATCH] =?UTF-8?q?fix(search):=20S1/W1/W3/W4/S3/S5/S2=20=E2=80=94?= =?UTF-8?q?=20required-term=20details,=20CAP=20note,=20restored=20guards?= =?UTF-8?q?=20(#529)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S1: fetchDetails now builds rank/highlight/matchedFields from the same [...positive, ...required] set matchedTerms uses, so a required-only query (`+кофейня`) no longer reports matchedFields:[] / rank:null. W1: documented that CANDIDATE_CAP bounds pagination reachability (JS-side), not query compute — a SQL LIMIT would corrupt the exact permission-filtered total. S2: one-line note that literal-`%`/`_` search was intentionally dropped (total:0). W3: restored the trgm-index EXPLAIN guard (search-lookup-explain.int-spec) that asserts the coalesce-free substring predicates use idx_pages_*_trgm, not Seq Scan. W4: added an integration ordering test proving title-exact > title-substring > text-only tier dominance under RRF. S3: added a test that an exact-title hit survives a tiny CANDIDATE_CAP window. S5: server pretest now also builds @docmost/mcp so the ToolWriteClass import resolves from clean CI (mirrors editor-ext/prosemirror-markdown). Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/server/package.json | 2 +- apps/server/src/core/search/search.service.ts | 35 ++++- .../integration/search-lexical.int-spec.ts | 76 +++++++++++ .../search-lookup-explain.int-spec.ts | 123 ++++++++++++++++++ 4 files changed, 229 insertions(+), 7 deletions(-) create mode 100644 apps/server/test/integration/search-lookup-explain.int-spec.ts diff --git a/apps/server/package.json b/apps/server/package.json index a6791d80..57193caf 100644 --- a/apps/server/package.json +++ b/apps/server/package.json @@ -23,7 +23,7 @@ "migration:reset": "tsx src/database/migrate.ts down-to NO_MIGRATIONS", "migration:codegen": "kysely-codegen --dialect=postgres --camel-case --env-file=../../.env --out-file=./src/database/types/db.d.ts", "lint": "eslint \"{src,apps,libs,test}/**/*.ts\" --fix", - "pretest": "pnpm --filter @docmost/editor-ext build && pnpm --filter @docmost/prosemirror-markdown build", + "pretest": "pnpm --filter @docmost/editor-ext build && pnpm --filter @docmost/prosemirror-markdown build && pnpm --filter @docmost/mcp build", "test": "jest", "test:int": "jest --config test/jest-integration.json", "test:watch": "jest --watch", diff --git a/apps/server/src/core/search/search.service.ts b/apps/server/src/core/search/search.service.ts index c3dc9442..325b3c9a 100644 --- a/apps/server/src/core/search/search.service.ts +++ b/apps/server/src/core/search/search.service.ts @@ -55,6 +55,11 @@ export function buildTsQuery(raw: string): string { // Escape the LIKE metacharacters (`%`, `_`, `\`) so every character is matched // LITERALLY by a `col LIKE '%' || q || '%'` predicate. Without this a query of // `%` or `_` would match every row. +// +// S2 (acceptance #10): escaping is intentionally SYMMETRIC on `%` and `_` — the +// ability to literal-search a bare `%` (or `_`) as a real character was +// deliberately dropped. Such a query parses to no positive recall and returns +// total:0 rather than matching everything. That trade-off is accepted. export function escapeLikePattern(raw: string): string { return (raw ?? '') .replace(/\\/g, '\\\\') @@ -361,6 +366,16 @@ export class SearchService { orderedIds = orderedIds.filter((id) => accessibleSet.has(id)); } + // W1 — CANDIDATE_CAP bounds pagination REACHABILITY (JS-side), NOT query + // compute. The `ranked` CTE deliberately has no SQL LIMIT: an exact `total` + // requires counting EVERY match, and the permission anti-join must run over + // the full match set, so a SQL LIMIT would corrupt `total`. The consequence + // is that a broad common term in a tenant with restricted pages sorts + + // anti-joins the whole match set on every request; the cap only truncates the + // reachable window afterwards (rows past it are unreachable → truncatedAtCap). + // Accepted for the small-tenant fork target — do NOT convert `cap` into a SQL + // LIMIT thinking it bounds compute; it does not, and doing so breaks the exact + // total contract (acceptance #8/#11). const total = orderedIds.length; const truncatedAtCap = total > cap; const window = orderedIds.slice(0, cap); @@ -438,7 +453,15 @@ export class SearchService { parsed: ParsedQuery, titleOnly: boolean, ): Promise { - const tsq = this.combineTsq(parsed.positive, parsed.mode); + // S1: rank/highlight/matchedFields must reflect required terms too, not just + // bare positives — a required-only query like `+кофейня` (no bare positive) + // really matches, so it must not report matchedFields:[] / rank:null. Build + // every detail expression from the SAME [...positive, ...required] set the + // matchedTerms path uses (`detailTerms` below). Excluded terms stay out (they + // are anti-matches). The candidate WHERE already enforced required-AND, so + // OR-combining them here is purely for display scoring/highlighting. + const detailTerms = [...parsed.positive, ...parsed.required]; + const tsq = this.combineTsq(detailTerms, parsed.mode); // rank/highlight are FTS-only (null for substring-only hits — the web already // falls back). ts_headline runs over the ORIGINAL text. const rankExpr = tsq @@ -475,7 +498,7 @@ export class SearchService { ); } } - for (const t of parsed.positive.filter((x) => x.branch === 'substring')) { + for (const t of detailTerms.filter((x) => x.branch === 'substring')) { const pat = this.likePattern(t); titleMatchParts.push( sql`LOWER(f_unaccent(pages.title)) LIKE ${pat} ESCAPE '\\'`, @@ -494,10 +517,10 @@ export class SearchService { : sql`false`; // Per-term matched flags (positive + required), assembled into matchedTerms. - // NB: aliases must be UNDERSCORE-FREE — the app's Kysely runs the - // CamelCasePlugin, which would rewrite a `t_0` result key to `t0`. `tmatch0` - // survives the round-trip unchanged. - const allTerms = [...parsed.positive, ...parsed.required]; + // Same set as `detailTerms` above. NB: aliases must be UNDERSCORE-FREE — the + // app's Kysely runs the CamelCasePlugin, which would rewrite a `t_0` result + // key to `t0`. `tmatch0` survives the round-trip unchanged. + const allTerms = detailTerms; const termCols = allTerms.map( (t, i) => sql`(${this.termMatchPred(t, titleOnly)}) AS "tmatch${sql.raw(String(i))}"`, diff --git a/apps/server/test/integration/search-lexical.int-spec.ts b/apps/server/test/integration/search-lexical.int-spec.ts index c8515252..fcffc151 100644 --- a/apps/server/test/integration/search-lexical.int-spec.ts +++ b/apps/server/test/integration/search-lexical.int-spec.ts @@ -385,4 +385,80 @@ describe('SearchService #529 lexical overhaul [integration]', () => { `.execute(db); expect(row.rows[0].m).toBe(true); }); + + // W4 — substring-tier dominance under RRF: title-exact (tier 3) > title- + // substring (tier 2) > text-only (tier 1). The engine encodes this via + // sub_tier DESC → rn_sub → RRF; no test asserted the end-to-end ordering, so + // this restores that guarantee. match:'substring' routes the term to the + // substring branch (no FTS leg), so sub_tier alone drives the order. + it('W4 tier dominance: title-exact > title-substring > text-only', async () => { + const exact = await insertPage({ title: 'tierdomxyz' }); // tier 3 + const titleSub = await insertPage({ + title: 'prefix tierdomxyz suffix', // tier 2 + }); + const textOnly = await insertPage({ + title: 'w4 unrelated heading', + textContent: 'body has tierdomxyz here', // tier 1 + }); + const res = await search(buildService(), { + query: 'tierdomxyz', + match: 'substring', + spaceId, + }); + const ids = res.items.map((i: any) => i.id); + expect(ids).toContain(exact); + expect(ids).toContain(titleSub); + expect(ids).toContain(textOnly); + // Strict tier order. + expect(ids.indexOf(exact)).toBeLessThan(ids.indexOf(titleSub)); + expect(ids.indexOf(titleSub)).toBeLessThan(ids.indexOf(textOnly)); + }); + + // S3 — an exact-title hit must NOT be lost when the match set exceeds + // CANDIDATE_CAP: it ranks first under RRF (tier 3) so it lands in the reachable + // window even with a tiny cap. Restores a guarantee the old lookup suite gave. + it('S3 exact-title survives the CANDIDATE_CAP window', async () => { + const exact = await insertPage({ title: 'capmarkerxyz' }); // tier 3 + for (let i = 0; i < 6; i++) { + await insertPage({ + title: `cap filler ${i}`, + textContent: 'noise capmarkerxyz noise', // tier 1 + }); + } + process.env.SEARCH_CANDIDATE_CAP = '2'; + try { + const res = await search(buildService(), { + query: 'capmarkerxyz', + match: 'substring', + spaceId, + limit: 25, + }); + // More matches than the cap → truncated, but the exact-title survives. + expect(res.total).toBeGreaterThan(2); + expect(res.truncatedAtCap).toBe(true); + expect(res.items.length).toBe(2); // the reachable window + expect(res.items.map((i: any) => i.id)).toContain(exact); + } finally { + delete process.env.SEARCH_CANDIDATE_CAP; + } + }); + + // S1 — a required-only query (`+term`, no bare positive) really matches, so its + // hits must report matchedFields / rank / highlight, not [] / null. Before the + // fix these detail exprs were built from parsed.positive only. + it('S1 required-only query populates matchedFields + rank on a title match', async () => { + const page = await insertPage({ + title: 'Кофейня s1маркер центр', + textContent: 'обычный текст без ключевого слова', + }); + const res = await search(buildService(), { query: '+кофейня', spaceId }); + const hit = res.items.find((i: any) => i.id === page); + expect(hit).toBeDefined(); + // The title matches the required term → matchedFields includes 'title'. + expect(hit.matchedFields).toContain('title'); + // FTS rank is populated (was null before the S1 fix). + expect(hit.rank).not.toBeNull(); + // matchedTerms already echoed the required term; still true. + expect(hit.matchedTerms).toContain('кофейня'); + }); }); diff --git a/apps/server/test/integration/search-lookup-explain.int-spec.ts b/apps/server/test/integration/search-lookup-explain.int-spec.ts new file mode 100644 index 00000000..4a50e553 --- /dev/null +++ b/apps/server/test/integration/search-lookup-explain.int-spec.ts @@ -0,0 +1,123 @@ +import { randomUUID } from 'node:crypto'; +import { Kysely, sql } from 'kysely'; +import { + getTestDb, + destroyTestDb, + createWorkspace, + createSpace, +} from './db'; + +/** + * #443 dead-index guard — EXPLAIN on the REAL DB. + * + * The lookup mode's substring predicates run a leading-wildcard + * `LOWER(f_unaccent(col)) LIKE '%q%'`. Those are only fast when Postgres uses + * the GIN trigram indexes: + * - idx_pages_title_trgm on (LOWER(f_unaccent(title))) [#348] + * - idx_pages_text_content_trgm on (LOWER(f_unaccent(text_content))) [#443] + * + * Postgres uses a functional index ONLY when the query expression matches the + * index expression EXACTLY. The original lookup query wrapped the columns in + * `coalesce(col,'')`, which differs from the coalesce-FREE index expression and + * silently forced a Seq Scan on pages for EVERY lookup (the MCP client always + * sends substring:true). This test locks that in. + * + * Discriminator: `SET enable_seqscan = off` asks the planner "CAN this predicate + * use the index at all?" — which is exactly what the coalesce bug breaks. With + * seqscan disabled: + * - the coalesce-FREE (fixed) predicate plans a Bitmap Index Scan on the trgm + * index (no Seq Scan on pages); + * - the coalesce-WRAPPED (buggy) predicate cannot use the index and falls back + * to a Seq Scan on pages even though seqscan is disabled. + * We assert both to prove the fix and to keep the regression from silently + * returning. + */ +describe('SearchService agent-lookup EXPLAIN — trgm index is live [integration]', () => { + let db: Kysely; + let workspaceId: string; + let spaceId: string; + + async function insertPage(title: string, textContent: string): Promise { + const id = randomUUID(); + await db + .insertInto('pages') + .values({ + id, + slugId: `slug-${id.slice(0, 12)}`, + title, + textContent, + spaceId, + workspaceId, + }) + .execute(); + } + + // Run EXPLAIN (no ANALYZE — we only inspect the chosen plan) and return the + // concatenated plan text. + async function explain(query: string): Promise { + const rows = await sql<{ 'QUERY PLAN': string }>`EXPLAIN ${sql.raw(query)}`.execute( + db, + ); + return (rows.rows as any[]).map((r) => r['QUERY PLAN']).join('\n'); + } + + beforeAll(async () => { + db = getTestDb(); + workspaceId = (await createWorkspace(db)).id; + spaceId = (await createSpace(db, workspaceId)).id; + + // Seed enough rows that a trigram index is a plausible plan. The content is + // varied so the '%needle%' pattern is selective. + for (let i = 0; i < 200; i++) { + await insertPage( + `seed-title-${i}`, + `seed body content number ${i} lorem ipsum dolor sit amet ${i}`, + ); + } + await insertPage('backup-srv.local', 'the needle-token-xyz lives here'); + + // Keep the trgm indexes' stats fresh so the planner costs them correctly. + await sql`ANALYZE pages`.execute(db); + }); + + afterAll(async () => { + await destroyTestDb(); + }); + + // Force the planner to answer "can the index be used?" rather than "is it + // cheaper than a seq scan on this size?". Restored after each test. + beforeEach(async () => { + await sql`SET enable_seqscan = off`.execute(db); + }); + afterEach(async () => { + await sql`RESET enable_seqscan`.execute(db); + }); + + it('title predicate (coalesce-FREE, as fixed) uses idx_pages_title_trgm, not a Seq Scan', async () => { + const plan = await explain( + `SELECT id FROM pages WHERE LOWER(f_unaccent(title)) LIKE '%srv.local%'`, + ); + expect(plan).toContain('idx_pages_title_trgm'); + expect(plan).not.toMatch(/Seq Scan on pages/i); + }); + + it('text_content predicate (coalesce-FREE, as fixed) uses idx_pages_text_content_trgm, not a Seq Scan', async () => { + const plan = await explain( + `SELECT id FROM pages WHERE LOWER(f_unaccent(text_content)) LIKE '%needle-token%'`, + ); + expect(plan).toContain('idx_pages_text_content_trgm'); + expect(plan).not.toMatch(/Seq Scan on pages/i); + }); + + // Negative control: the OLD coalesce-wrapped predicate must NOT be able to use + // the index — even with seqscan disabled it can only Seq Scan pages. If this + // ever stops seq-scanning, the coalesce/index expressions have re-aligned and + // the guard above is no longer meaningful. + it('coalesce-WRAPPED text predicate (the bug) cannot use the index — falls to Seq Scan', async () => { + const plan = await explain( + `SELECT id FROM pages WHERE LOWER(f_unaccent(coalesce(text_content,''))) LIKE '%needle-token%'`, + ); + expect(plan).not.toContain('idx_pages_text_content_trgm'); + expect(plan).toMatch(/Seq Scan on pages/i); + }); +});