From 0277db3d4905f5e733e6df205d5ac026de28620a Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Thu, 24 Sep 2026 09:27:14 +0200 Subject: [PATCH] fix(search): run both candidate legs in exact mode and page the window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The broad candidate leg was replaced with an empty array in `matchMode: "exact"`, so candidates came only from the server-side `subject ~ query` filter and any work package whose match lived in a description or a comment was unreachable. That inverts the parameter's own guidance: exact mode is advertised for literal strings known verbatim — error codes, ticket refs, commit SHAs — which are exactly the strings that live in descriptions and comments rather than subjects. Spec section 3.3 mandates two parallel requests with no exact-mode carve-out. Both legs now also page the candidate window in batches of 100 up to the unchanged 250 cap, the way meeting search already does. Every tool shape in this repo caps `pageSize` at 100 and OpenProject enforces a server-side maximum, so a single `pageSize: 250` request could silently return fewer candidates than the code claimed, with `response.total` ignored so nothing detected it. Also corrects `offset` on the search page from an item index to the 1-based page number its schema documents, and drops the duplicate `items` array from `WorkPackageSearchPage` (kept on `PaginatedResult` and the non-search list paths). The existing `workPackageFixtureClient` returns every element for any collection request and so cannot see a subject filter at all, which is why this was invisible to the suite. New tests use a fixture that honours the filter server-side; the old fixture is untouched. Co-Authored-By: Claude Opus 5 (1M context) --- src/services/work-packages.ts | 74 +++++++-- tests/services.test.ts | 285 +++++++++++++++++++++++++++++++++- 2 files changed, 344 insertions(+), 15 deletions(-) diff --git a/src/services/work-packages.ts b/src/services/work-packages.ts index da6d7bf..d6862b9 100644 --- a/src/services/work-packages.ts +++ b/src/services/work-packages.ts @@ -87,7 +87,13 @@ export interface WorkPackageSearchResult { snippet?: string; } -export interface WorkPackageSearchPage extends PaginatedResult { +/** + * Search results are exposed under `elements` only. `items` is deliberately + * omitted: the tool layer JSON-stringifies this whole object, so carrying the + * same array twice doubles the token cost of every search response. + */ +export interface WorkPackageSearchPage + extends Omit, "items"> { elements: WorkPackageSearchResult[]; degraded: boolean; enrichmentFailures: number; @@ -100,6 +106,13 @@ interface WorkPackageDeepContent { } const MAX_WORK_PACKAGE_CANDIDATES = 250; +/** + * OpenProject enforces a server-side maximum page size, and every tool shape + * in this repo caps `pageSize` at 100. Asking for 250 in a single request + * therefore risks silently receiving fewer candidates than the code assumes, + * so the candidate window is paged in batches instead. + */ +const CANDIDATE_BATCH_SIZE = 100; /** * Fetches the description and comment text for one work package. @@ -154,31 +167,63 @@ export async function searchWorkPackages( // results found". const listCandidates = async ( params: ListWorkPackagesParams - ): Promise<{ items: WorkPackageSummary[] }> => { + ): Promise<{ items: WorkPackageSummary[]; total: number }> => { try { return await listWorkPackages(params, opClient); } catch (error: unknown) { if (isFatalSearchError(error)) { throw error; } - return { items: [] }; + return { items: [], total: 0 }; } }; + /** + * Accumulates one candidate leg in server-acceptable batches, up to the + * shared 250 cap. Stops once the server says the collection is exhausted + * (`collected.length >= total`) or hands back a short page. + */ + const collectLeg = async ( + params: ListWorkPackagesParams + ): Promise => { + const collected: WorkPackageSummary[] = []; + let offset = 1; + + while (collected.length < MAX_WORK_PACKAGE_CANDIDATES) { + const page = await listCandidates({ + ...params, + pageSize: CANDIDATE_BATCH_SIZE, + offset, + }); + + if (page.items.length === 0) { + break; + } + collected.push(...page.items); + + if (collected.length >= page.total || page.items.length < CANDIDATE_BATCH_SIZE) { + break; + } + offset += 1; + } + + return collected.slice(0, MAX_WORK_PACKAGE_CANDIDATES); + }; + + // Both legs run in every match mode. Exact mode must not be restricted to + // the server-side subject filter: the literal strings a caller reaches for + // exact mode with (error codes, ticket refs, commit SHAs) overwhelmingly + // live in descriptions and comments, which only the broad leg can surface. const fetchCandidates = async (): Promise => { const [precise, broad] = await Promise.all([ - listCandidates({ ...sharedFilters, subject: query, pageSize: MAX_WORK_PACKAGE_CANDIDATES }), - matchMode === "exact" - ? Promise.resolve({ items: [] as WorkPackageSummary[] }) - : listCandidates({ - ...sharedFilters, - pageSize: MAX_WORK_PACKAGE_CANDIDATES, - sortBy: '[["updatedAt","desc"]]', - }), + collectLeg({ ...sharedFilters, subject: query }), + collectLeg({ ...sharedFilters, sortBy: '[["updatedAt","desc"]]' }), ]); + // Precise leg first: on a collision the exact subject match wins the slot, + // so exact matches always survive the 250 cap. const byId = new Map(); - for (const item of [...precise.items, ...broad.items]) { + for (const item of [...precise, ...broad]) { byId.set(item.id, item); } return Array.from(byId.values()).slice(0, MAX_WORK_PACKAGE_CANDIDATES); @@ -213,9 +258,11 @@ export async function searchWorkPackages( snippet: entry.matches[0]?.snippet, })); + // `offset` is a 1-based PAGE number, as the tool schema documents and as the + // rest of this codebase and the OpenProject API treat it — not an item index. const offset = options?.offset ?? 1; const pageSize = options?.pageSize ?? 20; - const startIndex = Math.max(0, offset - 1); + const startIndex = Math.max(0, offset - 1) * pageSize; const paged = all.slice(startIndex, startIndex + pageSize); return { @@ -224,7 +271,6 @@ export async function searchWorkPackages( pageSize, offset, elements: paged, - items: paged, degraded: result.degraded, enrichmentFailures: result.enrichmentFailures, }; diff --git a/tests/services.test.ts b/tests/services.test.ts index 978b29a..7704869 100644 --- a/tests/services.test.ts +++ b/tests/services.test.ts @@ -693,7 +693,7 @@ describe("Live Container Integration (Domain Services)", () => { expect(firstWp.subject).toBeDefined(); const searchRes = await domainServices.searchWorkPackages("MCP", { projectId: "mcp-test-project" }, client); - expect(searchRes.items.length).toBeGreaterThan(0); + expect(searchRes.elements.length).toBeGreaterThan(0); }); runLiveTests("live: queries service operations", async () => { @@ -941,3 +941,286 @@ describe("Work Package Fuzzy Search", () => { }); }); + +/** + * These fixtures deliberately HONOUR the server-side `subject ~` filter. + * `workPackageFixtureClient` above returns every element for any collection + * request, which is why the exact-mode candidate starvation bug was invisible + * to the suite: a subject-filtered leg that returns the whole corpus looks + * identical to a broad leg. + */ +describe("Work Package Search Candidate Union", () => { + interface FixtureElement { + _type: string; + id: number; + subject: string; + _links: Record; + updatedAt: string; + } + + function makeElement(id: number, subject: string, updatedAt: string): FixtureElement { + return { + _type: "WorkPackage", + id, + subject, + _links: { + type: { title: "Bug" }, + status: { title: "New" }, + project: { href: "/api/v3/projects/1", title: "Demo project" }, + }, + updatedAt, + }; + } + + /** Reads the `subject ~` value out of the serialized filter payload. */ + function subjectFilterOf(query?: Record): string | undefined { + const raw = query?.filters; + if (typeof raw !== "string") return undefined; + const filters = JSON.parse(raw) as Array>; + for (const filter of filters) { + const subject = filter.subject; + if (subject) return subject.values?.[0]; + } + return undefined; + } + + /** + * A fixture client that applies the subject filter server-side, the way a + * real OpenProject instance does. + */ + function subjectAwareClient( + elements: FixtureElement[], + deep: Record, + calls: Array<{ path: string; query?: Record }> = [] + ) { + return { + get: async (path: string, query?: Record) => { + calls.push({ path, query }); + + if (path.startsWith("work_packages/") || path.startsWith("/api/v3/work_packages/")) { + const id = Number(path.match(/work_packages\/(\d+)/)?.[1] ?? NaN); + if (path.includes("activities")) { + const comment = deep[id]?.comment; + return { + _type: "Collection", + _embedded: { + elements: comment + ? [ + { + id: id * 10, + version: 1, + createdAt: "2026-09-11T00:00:00Z", + comment: { raw: comment }, + _links: { user: { href: "/api/v3/users/4", title: "Admin" } }, + }, + ] + : [], + }, + }; + } + const match = elements.find((e) => e.id === id); + return { ...match, description: { raw: deep[id]?.description ?? "" } }; + } + + const subject = subjectFilterOf(query); + const matched = + subject === undefined + ? elements + : elements.filter((e) => + e.subject.toLowerCase().includes(subject.toLowerCase()) + ); + + return { + _type: "Collection", + total: matched.length, + count: matched.length, + pageSize: 100, + offset: 1, + _embedded: { elements: matched }, + }; + }, + } as unknown as OpenProjectClient; + } + + const elements = [ + makeElement(1, "Fix login redirect", "2026-09-10T00:00:00Z"), + makeElement(2, "Update dependencies", "2026-09-11T00:00:00Z"), + ]; + const deep = { + 1: { description: "Session cookie is dropped" }, + 2: { comment: "Blocked by error ERR_TLS_EXPIRED on deploy" }, + }; + + test("the subject-aware fixture really does filter (guards the tests below)", async () => { + const result = await searchWorkPackages( + "Update dependencies", + { matchMode: "exact" }, + subjectAwareClient(elements, deep) + ); + expect(result.elements.map((e) => e.workPackage.id)).toEqual([2]); + }); + + test("exact mode finds a comment-only match the subject filter cannot return", async () => { + const result = await searchWorkPackages( + "ERR_TLS_EXPIRED", + { matchMode: "exact" }, + subjectAwareClient(elements, deep) + ); + expect(result.elements).toHaveLength(1); + expect(result.elements[0]!.workPackage.id).toBe(2); + expect(result.elements[0]!.matchedFields).toContain("comments"); + }); + + test("exact mode finds a description-only match the subject filter cannot return", async () => { + const result = await searchWorkPackages( + "Session cookie", + { matchMode: "exact" }, + subjectAwareClient(elements, deep) + ); + expect(result.elements).toHaveLength(1); + expect(result.elements[0]!.workPackage.id).toBe(1); + expect(result.elements[0]!.matchedFields).toContain("description"); + }); + + test("exact mode still issues both the precise and the broad candidate leg", async () => { + const calls: Array<{ path: string; query?: Record }> = []; + await searchWorkPackages( + "login", + { matchMode: "exact" }, + subjectAwareClient(elements, deep, calls) + ); + const collectionCalls = calls.filter((c) => c.path === "work_packages"); + expect(collectionCalls.some((c) => subjectFilterOf(c.query) === "login")).toBe(true); + expect(collectionCalls.some((c) => subjectFilterOf(c.query) === undefined)).toBe(true); + }); + + test("never requests a page size above the documented 100 maximum", async () => { + const calls: Array<{ path: string; query?: Record }> = []; + await searchWorkPackages("login", {}, subjectAwareClient(elements, deep, calls)); + const pageSizes = calls + .filter((c) => c.path === "work_packages") + .map((c) => Number(c.query?.pageSize)); + expect(pageSizes.length).toBeGreaterThan(0); + for (const pageSize of pageSizes) { + expect(pageSize).toBeLessThanOrEqual(100); + } + }); + + test("pages the candidate window until the server-reported total is collected", async () => { + const requestedOffsets: number[] = []; + // 150 candidates behind a server that hands back 100 per page: a single + // pageSize=250 request would have been capped at 100 by a real instance. + const corpus = Array.from({ length: 150 }, (_, i) => + makeElement(i + 1, `Candidate ${i + 1}`, "2026-09-10T00:00:00Z") + ); + + const client = { + get: async (path: string, query?: Record) => { + if (path.startsWith("work_packages/") || path.startsWith("/api/v3/work_packages/")) { + const id = Number(path.match(/work_packages\/(\d+)/)?.[1] ?? NaN); + if (path.includes("activities")) { + return { _type: "Collection", _embedded: { elements: [] } }; + } + return { ...corpus.find((e) => e.id === id), description: { raw: "" } }; + } + const pageSize = Number(query?.pageSize ?? 20); + expect(pageSize).toBeLessThanOrEqual(100); + const offset = Number(query?.offset ?? 1); + if (subjectFilterOf(query) === undefined) { + requestedOffsets.push(offset); + } + const start = (offset - 1) * pageSize; + const page = corpus.slice(start, start + pageSize); + return { + _type: "Collection", + total: corpus.length, + count: page.length, + pageSize, + offset, + _embedded: { elements: page }, + }; + }, + } as unknown as OpenProjectClient; + + const result = await searchWorkPackages("Candidate 137", {}, client); + expect(requestedOffsets).toEqual([1, 2]); + // Candidate 137 lives on the second page; without paging it is unreachable. + expect(result.elements[0]!.workPackage.id).toBe(137); + }); +}); + +describe("Work Package Search Response Shape", () => { + function pagedFixtureClient(count: number) { + const elements = Array.from({ length: count }, (_, i) => ({ + _type: "WorkPackage", + id: i + 1, + subject: `Release checklist item ${i + 1}`, + _links: { + type: { title: "Task" }, + status: { title: "New" }, + project: { href: "/api/v3/projects/1", title: "Demo project" }, + }, + updatedAt: "2026-09-10T00:00:00Z", + })); + + return { + get: async (path: string) => { + if (path.startsWith("work_packages/") || path.startsWith("/api/v3/work_packages/")) { + const id = Number(path.match(/work_packages\/(\d+)/)?.[1] ?? NaN); + if (path.includes("activities")) { + return { _type: "Collection", _embedded: { elements: [] } }; + } + return { ...elements.find((e) => e.id === id), description: { raw: "" } }; + } + return { + _type: "Collection", + total: elements.length, + count: elements.length, + pageSize: 100, + offset: 1, + _embedded: { elements }, + }; + }, + } as unknown as OpenProjectClient; + } + + test("offset is a page number: page 2 does not overlap page 1", async () => { + const client = pagedFixtureClient(12); + const page1 = await searchWorkPackages( + "Release checklist item", + { offset: 1, pageSize: 5 }, + client + ); + const page2 = await searchWorkPackages( + "Release checklist item", + { offset: 2, pageSize: 5 }, + client + ); + + const whole = await searchWorkPackages( + "Release checklist item", + { offset: 1, pageSize: 100 }, + client + ); + const allIds = whole.elements.map((e) => e.workPackage.id); + const ids1 = page1.elements.map((e) => e.workPackage.id); + const ids2 = page2.elements.map((e) => e.workPackage.id); + + expect(allIds).toHaveLength(12); + expect(ids1).toEqual(allIds.slice(0, 5)); + // The bug returned allIds.slice(1, 6) here — four of five items repeated. + expect(ids2).toEqual(allIds.slice(5, 10)); + expect(ids1.filter((id) => ids2.includes(id))).toEqual([]); + }); + + test("does not ship the result array twice", async () => { + const result = await searchWorkPackages( + "Release checklist item", + {}, + pagedFixtureClient(3) + ); + expect(result.elements).toHaveLength(3); + expect(Object.keys(result)).not.toContain("items"); + expect(JSON.parse(JSON.stringify(result))).not.toHaveProperty("items"); + }); +});