diff --git a/apps/mobile/app/book/[slug].tsx b/apps/mobile/app/book/[slug].tsx index abb00edb6..8e433f5f2 100644 --- a/apps/mobile/app/book/[slug].tsx +++ b/apps/mobile/app/book/[slug].tsx @@ -552,6 +552,11 @@ export default function BookDetailScreen() { title={book.title} author={book.authors.map(a => a.name).join(', ') || null} editionId={book.id} + // Both identifiers: the read tools are slug-keyed, the insight tools are editionId-keyed. + slug={book.slug} + // Already fetched for Continue Reading; the assistant was told none of it until now. + progressFraction={continuePct} + chapterTitle={book.chapters.find(c => c.slug === continueSlug)?.title ?? null} /> )} diff --git a/apps/web/src/components/library/__tests__/DiscussWithAssistant.test.tsx b/apps/web/src/components/library/__tests__/DiscussWithAssistant.test.tsx index 16e3b7d0d..6dbdd504f 100644 --- a/apps/web/src/components/library/__tests__/DiscussWithAssistant.test.tsx +++ b/apps/web/src/components/library/__tests__/DiscussWithAssistant.test.tsx @@ -31,15 +31,20 @@ describe('DiscussWithAssistant', () => { } }) - it('names an upload by bookId and a catalog book by editionId, never both', () => { + it('names an upload by bookId, and a catalog book by BOTH slug and editionId', () => { const { unmount } = render() expect(decodeURIComponent(hrefOf('library.discuss.claude'))).toContain('bookId b-1') expect(decodeURIComponent(hrefOf('library.discuss.claude'))).not.toContain('editionId') unmount() - render() - expect(decodeURIComponent(hrefOf('library.discuss.claude'))).toContain('editionId e-1') - expect(decodeURIComponent(hrefOf('library.discuss.claude'))).not.toContain('bookId') + // A catalog book needs both: get_book/get_chapter are slug-keyed, the insight tools are + // editionId-keyed. Sending only the id named tools that reject every call made from it, which + // is why the catalog button did not work at all until 2026-09-10. + render() + const brief = decodeURIComponent(hrefOf('library.discuss.claude')) + expect(brief).toContain('editionId e-1') + expect(brief).toContain('"dracula"') + expect(brief).not.toContain('bookId') }) it('reads progress as a fraction, not as a percentage', () => { diff --git a/apps/web/src/pages/BookDetailPage.tsx b/apps/web/src/pages/BookDetailPage.tsx index 4fa8b1be6..be570f423 100644 --- a/apps/web/src/pages/BookDetailPage.tsx +++ b/apps/web/src/pages/BookDetailPage.tsx @@ -363,6 +363,12 @@ export function BookDetailPage() { title={book.title} author={book.authors.map(a => a.name).join(', ') || null} editionId={book.id} + // A catalog book needs BOTH: get_book/get_chapter take the slug, the insight tools take + // the editionId. Sending only the id named tools that would reject every call. + slug={book.slug} + // The screen already knows where the reader stopped — the Continue Reading button above + // is built from it — and used to hand the assistant none of it. + chapterTitle={book.chapters.find(c => c.slug === continueSlug)?.title ?? null} /> )} diff --git a/packages/shared/src/lib/assistantHandoff.test.ts b/packages/shared/src/lib/assistantHandoff.test.ts index faa075bf8..7eb1d7237 100644 --- a/packages/shared/src/lib/assistantHandoff.test.ts +++ b/packages/shared/src/lib/assistantHandoff.test.ts @@ -31,16 +31,90 @@ describe('buildHandoffBrief', () => { expect(brief).not.toContain('editionId') }) - it('names a catalog book by editionId and points at the catalog tools', () => { + /** + * Which identifier each tool actually accepts, mirroring the JSON schemas in + * `McpToolCatalog` — all of which set `additionalProperties: false`, so passing the wrong one is + * a rejected call rather than an ignored field. + * + * This table is the point of the test below. The previous version asserted that the brief string + * contained the substring "get_book" — prose checked against prose — and therefore passed happily + * while the brief handed `get_book` an editionId it cannot take. The catalog Discuss button did + * not work at all, and no test noticed. Keep this in step with the schemas. + */ + const TOOL_IDENTIFIER: Record> = { + get_book: ['slug'], + get_chapter: ['slug'], + get_my_book: ['bookId'], + get_my_chapter: ['bookId'], + // The insight tools take EITHER, XOR, keyed by book type: bookId for an upload, editionId for a + // catalog book. So they are satisfied by whichever one the brief is carrying. + get_my_insights: ['bookId', 'editionId'], + save_insight: ['bookId', 'editionId'], + } + + /** Every tool the brief names, in order of appearance. */ + const toolsNamedIn = (brief: string) => + Object.keys(TOOL_IDENTIFIER).filter(name => brief.includes(name)) + + it('gives a catalog book BOTH identifiers, each next to the tools that take it', () => { const brief = buildHandoffBrief({ title: 'Dracula', editionId: '33333333-3333-3333-3333-333333333333', + slug: 'dracula', }) + + // Read tools are slug-keyed; insight tools are editionId-keyed. A brief carrying only one of + // the two names tools that would reject every call made from it. + expect(brief).toContain('"dracula"') expect(brief).toContain('editionId 33333333-3333-3333-3333-333333333333') - expect(brief).toContain('get_book') + expect(toolsNamedIn(brief)).toContain('get_book') + expect(toolsNamedIn(brief)).toContain('save_insight') expect(brief).not.toContain('bookId') }) + it('never names a tool without the identifier that tool accepts', () => { + const cases = [ + { + what: 'catalog', + book: { title: 'Dracula', editionId: '33333333-3333-3333-3333-333333333333', slug: 'dracula' }, + present: { slug: '"dracula"', editionId: 'editionId 33333333-3333-3333-3333-333333333333' }, + }, + { + what: 'upload', + book: { title: 'My PDF', bookId: '22222222-2222-2222-2222-222222222222' }, + present: { bookId: 'bookId 22222222-2222-2222-2222-222222222222' }, + }, + ] as const + + for (const c of cases) { + const brief = buildHandoffBrief(c.book) + const named = toolsNamedIn(brief) + expect(named.length).toBeGreaterThan(0) + + for (const tool of named) { + const accepts = TOOL_IDENTIFIER[tool] + const carried = accepts.some(id => { + const marker = (c.present as Record)[id] + return marker !== undefined && brief.includes(marker) + }) + expect( + carried, + `${c.what} brief names ${tool}, which takes ${accepts.join(' or ')}, but carries none of them`, + ).toBe(true) + } + } + }) + + it('says nothing about the connector when a catalog book has no slug', () => { + // Half the pair is worse than none: it would name slug-keyed tools with nothing to give them. + const brief = buildHandoffBrief({ + title: 'Dracula', + editionId: '33333333-3333-3333-3333-333333333333', + }) + expect(brief).not.toContain('get_book') + expect(brief).not.toContain('editionId') + }) + it('asks the assistant to write conclusions back', () => { // Without this line the conversation happens and nothing comes home, which is // the entire failure mode the feature exists to fix. diff --git a/packages/shared/src/lib/assistantHandoff.ts b/packages/shared/src/lib/assistantHandoff.ts index 037301af9..a39802cd0 100644 --- a/packages/shared/src/lib/assistantHandoff.ts +++ b/packages/shared/src/lib/assistantHandoff.ts @@ -34,6 +34,15 @@ export interface HandoffBook { bookId?: string /** Edition id for a catalog book. */ editionId?: string + /** + * The catalog book's slug. Required alongside `editionId`, not instead of it: a catalog book needs + * BOTH identifiers because the tools disagree about which one they take. `get_book` and + * `get_chapter` are keyed by slug (`"required": ["slug"]`, `additionalProperties: false`), while + * `save_insight` and `get_my_insights` are keyed by `editionId`. Handing an assistant only the + * editionId — which this did until 2026-09-10 — produced a brief naming tools that would reject + * every call made from it, so the catalog Discuss button never worked at all. + */ + slug?: string /** * How far in, as a FRACTION of the book: 0..1, the way progress is stored * everywhere else in this codebase ("the server stores a book-wide fraction", @@ -76,19 +85,22 @@ export function buildHandoffBrief(book: HandoffBook): string { // The identifier and the tool names. A connected client acts on this; an // unconnected one ignores it and the conversation still works. - const id = book.bookId - ? `bookId ${book.bookId}` - : book.editionId - ? `editionId ${book.editionId}` - : null - if (id) { + // An upload is addressed by one id everywhere. A catalog book is not: the read tools take a slug + // and the insight tools take an editionId, so the brief has to carry both and say which is which. + if (book.bookId) { + lines.push('') + lines.push( + `If you have the TextStack connector, this book is bookId ${book.bookId}. ` + + 'Read it with get_my_book and get_my_chapter, and check get_my_insights first in case we have ' + + 'discussed it before. When we are done, write the conclusions back with save_insight so I find ' + + 'them in the book later.') + } else if (book.editionId && book.slug) { lines.push('') lines.push( - `If you have the TextStack connector, this book is ${id}. ` + - (book.bookId - ? 'Read it with get_my_book and get_my_chapter, and check get_my_insights first in case we have discussed it before. ' - : 'Read it with get_book and get_chapter, and check get_my_insights first in case we have discussed it before. ') - + 'When we are done, write the conclusions back with save_insight so I find them in the book later.') + `If you have the TextStack connector: read this book with get_book and get_chapter using slug ` + + `"${book.slug}", and use editionId ${book.editionId} for get_my_insights and save_insight. ` + + 'Check get_my_insights first in case we have discussed it before, and write the conclusions ' + + 'back with save_insight when we are done so I find them in the book later.') } const brief = lines.join('\n')