fix(meetings): page-number offset, author match type, and auth propagation
Three fixes to the meetings service: - `offset` was documented as a 1-based page number but implemented as an item index, so with `pageSize: 20` a request for page 2 returned items 2-21 — nineteen of the twenty the caller already had. This changes the observable behaviour of `openproject_search_meetings`: paging now returns disjoint pages. Shipping two identical pagination bugs with only the work package one fixed would have been worse. - `author` was added as a shallow field but never to the `matchType` union, so `resolveMatchType` fell through and reported a meeting found by a person's name as having matched on project name. - `getMeeting` retained `.catch(() => undefined)` on its agenda-item fetch, swallowing 401/403/429. This is the tool a model calls immediately after a search hit, so an expired token would have been reported as "this meeting has no agenda items". Also drops the duplicate `items` array from `MeetingSearchPage`; the `items` field on `PaginatedResult` and on `listMeetings` is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
0277db3d49
commit
216b460c56
@@ -7,7 +7,7 @@ import type { OpenProjectClient } from "../client/api-client.ts";
|
||||
import { extractIdFromHref, extractRawText } from "../client/hal-parser.ts";
|
||||
import type { HalCollection, HalResource } from "../client/types.ts";
|
||||
import { resolveClient, resolveProjectId } from "./helper.ts";
|
||||
import { searchPipeline } from "../search/pipeline.ts";
|
||||
import { isFatalSearchError, searchPipeline } from "../search/pipeline.ts";
|
||||
import { rankRecords } from "../search/rank.ts";
|
||||
import type { FieldSpec, MatchMode } from "../search/rank.ts";
|
||||
|
||||
@@ -51,7 +51,7 @@ export interface MeetingDetail extends MeetingSummary {
|
||||
|
||||
export interface MeetingSearchResult {
|
||||
meeting: MeetingSummary;
|
||||
matchType: "title" | "location" | "agenda_item" | "participant" | "project";
|
||||
matchType: "title" | "location" | "agenda_item" | "participant" | "author" | "project";
|
||||
matchedAgendaItems?: Array<{
|
||||
id: number;
|
||||
title: string;
|
||||
@@ -72,7 +72,14 @@ export interface PaginatedResult<T> {
|
||||
items?: T[];
|
||||
}
|
||||
|
||||
export interface MeetingSearchPage extends PaginatedResult<MeetingSearchResult> {
|
||||
/**
|
||||
* 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 MeetingSearchPage
|
||||
extends Omit<PaginatedResult<MeetingSearchResult>, "items"> {
|
||||
elements: MeetingSearchResult[];
|
||||
/** True when some deep content could not be read. */
|
||||
degraded: boolean;
|
||||
enrichmentFailures: number;
|
||||
@@ -348,7 +355,15 @@ export async function getMeeting(
|
||||
meetingPromise,
|
||||
opClient
|
||||
.get<HalCollection<Record<string, unknown>>>(`/api/v3/meetings/${id}/agenda_items`)
|
||||
.catch(() => undefined),
|
||||
.catch((error: unknown) => {
|
||||
// An expired token or a rate limit must not be reported to the
|
||||
// model as "this meeting has no agenda items". Degrade only on
|
||||
// errors that genuinely mean "agenda unavailable" (404, 500, ...).
|
||||
if (isFatalSearchError(error)) {
|
||||
throw error;
|
||||
}
|
||||
return undefined;
|
||||
}),
|
||||
]);
|
||||
|
||||
let agendaItems: AgendaItem[] | undefined;
|
||||
@@ -410,6 +425,11 @@ function resolveMatchType(matchedFields: string[]): MeetingSearchResult["matchTy
|
||||
return "agenda_item";
|
||||
}
|
||||
if (matchedFields.includes("participant")) return "participant";
|
||||
// `author` sits below `participant` and above the `project` fallback: both
|
||||
// are person matches, and author carries the lower field weight. Without
|
||||
// this branch an author-only hit falls through and is reported to the model
|
||||
// as a project-name match.
|
||||
if (matchedFields.includes("author")) return "author";
|
||||
return "project";
|
||||
}
|
||||
|
||||
@@ -533,9 +553,11 @@ export async function searchMeetings(
|
||||
};
|
||||
});
|
||||
|
||||
// `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 = params.offset ?? 1;
|
||||
const pageSize = params.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 {
|
||||
@@ -544,7 +566,6 @@ export async function searchMeetings(
|
||||
pageSize,
|
||||
offset,
|
||||
elements: paged,
|
||||
items: paged,
|
||||
degraded: result.degraded,
|
||||
enrichmentFailures: result.enrichmentFailures,
|
||||
};
|
||||
|
||||
@@ -577,3 +577,149 @@ describe("Meetings Fuzzy Search", () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Meetings Search Regressions", () => {
|
||||
function meetingResource(
|
||||
id: number,
|
||||
title: string,
|
||||
extra: Record<string, unknown> = {},
|
||||
links: Record<string, unknown> = {}
|
||||
) {
|
||||
return {
|
||||
_type: "Meeting",
|
||||
id,
|
||||
title,
|
||||
state: "open",
|
||||
startTime: `2026-09-${String(id).padStart(2, "0")}T10:00:00Z`,
|
||||
endTime: `2026-09-${String(id).padStart(2, "0")}T11:00:00Z`,
|
||||
_links: { project: { href: "/api/v3/projects/1", title: "Demo" }, ...links },
|
||||
...extra,
|
||||
};
|
||||
}
|
||||
|
||||
function collectionClient(elements: unknown[]) {
|
||||
return {
|
||||
get: async (path: string) => {
|
||||
if (path.startsWith("/api/v3/meetings?") || path === "/api/v3/meetings") {
|
||||
return {
|
||||
_type: "Collection",
|
||||
total: elements.length,
|
||||
count: elements.length,
|
||||
pageSize: 100,
|
||||
offset: 1,
|
||||
_embedded: { elements },
|
||||
};
|
||||
}
|
||||
if (path.includes("/agenda_items")) {
|
||||
return { _type: "Collection", total: 0, _embedded: { elements: [] } };
|
||||
}
|
||||
const id = Number(path.match(/meetings\/(\d+)/)?.[1] ?? NaN);
|
||||
return elements.find((e) => (e as { id: number }).id === id);
|
||||
},
|
||||
} as unknown as OpenProjectClient;
|
||||
}
|
||||
|
||||
test("an author-only match is reported as an author match, not a project match", async () => {
|
||||
const client = collectionClient([
|
||||
meetingResource(
|
||||
1,
|
||||
"Weekly sync",
|
||||
{},
|
||||
{ author: { href: "/api/v3/users/7", title: "Ingrid Kowalczyk" } }
|
||||
),
|
||||
meetingResource(
|
||||
2,
|
||||
"Retrospective",
|
||||
{},
|
||||
{ author: { href: "/api/v3/users/8", title: "Sam Okafor" } }
|
||||
),
|
||||
]);
|
||||
|
||||
const res = await searchMeetings({ query: "Kowalczyk" }, client);
|
||||
expect(res.elements).toHaveLength(1);
|
||||
expect(res.elements[0]!.meeting.id).toBe(1);
|
||||
expect(res.elements[0]!.matchedFields).toContain("author");
|
||||
// Before the fix this fell through resolveMatchType and reported
|
||||
// "project", telling the model the meeting matched a project name.
|
||||
expect(res.elements[0]!.matchType).toBe("author");
|
||||
});
|
||||
|
||||
test("offset is a page number: page 2 does not overlap page 1", async () => {
|
||||
const elements = Array.from({ length: 12 }, (_, i) =>
|
||||
meetingResource(i + 1, `Arch Review ${i + 1}`)
|
||||
);
|
||||
const client = collectionClient(elements);
|
||||
|
||||
const whole = await searchMeetings(
|
||||
{ query: "Arch Review", offset: 1, pageSize: 100 },
|
||||
client
|
||||
);
|
||||
const page1 = await searchMeetings({ query: "Arch Review", offset: 1, pageSize: 5 }, client);
|
||||
const page2 = await searchMeetings({ query: "Arch Review", offset: 2, pageSize: 5 }, client);
|
||||
|
||||
const allIds = whole.elements.map((e) => e.meeting.id);
|
||||
const ids1 = page1.elements.map((e) => e.meeting.id);
|
||||
const ids2 = page2.elements.map((e) => e.meeting.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 client = collectionClient([meetingResource(1, "Arch Review")]);
|
||||
const res = await searchMeetings({ query: "Arch Review" }, client);
|
||||
expect(res.elements).toHaveLength(1);
|
||||
expect(Object.keys(res)).not.toContain("items");
|
||||
expect(JSON.parse(JSON.stringify(res))).not.toHaveProperty("items");
|
||||
});
|
||||
});
|
||||
|
||||
describe("getMeeting error propagation", () => {
|
||||
function clientFailingAgendaWith(error: unknown) {
|
||||
return {
|
||||
get: async (path: string) => {
|
||||
if (path.includes("/agenda_items")) {
|
||||
throw error;
|
||||
}
|
||||
return {
|
||||
_type: "Meeting",
|
||||
id: 5,
|
||||
title: "Weekly sync",
|
||||
state: "open",
|
||||
startTime: "2026-09-05T10:00:00Z",
|
||||
endTime: "2026-09-05T11:00:00Z",
|
||||
_links: { project: { href: "/api/v3/projects/1", title: "Demo" } },
|
||||
};
|
||||
},
|
||||
} as unknown as OpenProjectClient;
|
||||
}
|
||||
|
||||
test("propagates an authentication failure from the agenda fetch", async () => {
|
||||
await expect(
|
||||
getMeeting(5, { includeAgendaItems: true }, clientFailingAgendaWith(
|
||||
new OpenProjectAuthenticationError()
|
||||
))
|
||||
).rejects.toThrow(OpenProjectAuthenticationError);
|
||||
});
|
||||
|
||||
test("propagates a rate limit from the agenda fetch", async () => {
|
||||
await expect(
|
||||
getMeeting(5, { includeAgendaItems: true }, clientFailingAgendaWith(
|
||||
new OpenProjectError("rate limited", { statusCode: 429 })
|
||||
))
|
||||
).rejects.toThrow("rate limited");
|
||||
});
|
||||
|
||||
test("still degrades to an empty agenda on a non-fatal failure", async () => {
|
||||
const detail = await getMeeting(
|
||||
5,
|
||||
{ includeAgendaItems: true },
|
||||
clientFailingAgendaWith(new OpenProjectError("boom", { statusCode: 500 }))
|
||||
);
|
||||
expect(detail.id).toBe(5);
|
||||
expect(detail.agendaItems).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user