The catalog Discuss button never worked - #599
Merged
Merged
Conversation
It handed the assistant an `editionId` and told it to call `get_book` and `get_chapter` — both of which are keyed by `slug` with `additionalProperties: false`, so every call made from that brief was rejected. The button has been shipping in that state since it was written. A catalog book needs BOTH identifiers, because the tools disagree about which one they take: the read tools are slug-keyed, `save_insight` and `get_my_insights` are editionId-keyed. The brief now carries both and says which is for what. With only one of the pair it says nothing about the connector at all — half the pair is worse than none, because it names tools it cannot satisfy. Its test could not have caught this: it asserted the brief *string* contained the substring "get_book", which is prose checked against prose. Replaced with a table of which identifier each tool accepts — mirroring the JSON schemas in McpToolCatalog — and an assertion that the brief never names a tool without carrying an identifier that tool takes. Writing that table caught my own error in it: the insight tools take either id, XOR, not editionId only. Same two call sites also stopped throwing away what they already knew. Both catalog screens compute the reader's position for their own Continue Reading button and handed the assistant none of it; they now pass the chapter title, and mobile the progress fraction as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The brief handed the assistant an
editionIdand told it to callget_bookandget_chapter. Both are keyed byslug, withadditionalProperties: false— soevery call made from that brief was rejected. The button has been shipping in
that state since it was written, which is part of why zero conclusions ever came
back.
The fix
A catalog book needs both identifiers, because the tools disagree about which
one they take:
get_book,get_chapterslugsave_insight,get_my_insightseditionId(orbookId, XOR, by book type)The brief now carries both and says which is for what. With only one of the pair
it says nothing about the connector at all — half the pair is worse than none,
because it names tools it cannot satisfy.
Uploads are unchanged: one id addresses everything.
Its test could not have caught this
The old assertion was that the brief string contained the substring
get_book— prose checked against prose. It passed happily while the brief named a tool it
was feeding the wrong identifier.
Replaced with a table of which identifier each tool accepts, mirroring the JSON
schemas in
McpToolCatalog, plus an assertion that the brief never names a toolwithout carrying an identifier that tool takes. Writing the table caught my own
error in it: the insight tools accept either id, XOR, not
editionIdonly.Same two call sites, second fix
Both catalog screens already compute where the reader stopped — the Continue
Reading button above the Discuss button is built from it — and handed the
assistant none of it. They now pass the chapter title, and mobile the progress
fraction as well. That closes one of the carry-forward items in
assistant-handoff.md.Verified
698 web, 369 mobile, 12 shared tests; tsc clean on web and mobile.
🤖 Generated with Claude Code
https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E