Library: five tools an agent may hold a library by - #221
Conversation
The backend already knew how to do this: a tag has a name, a description, a colour and its own order of books, and a book has a note and a rating. What was missing was a key a machine may hold and a wrapper that reads as tools. Books and tags are named the way a person names them, by title or by word, and a word that matches two books is refused with both rather than guessed at. A partial order is enough: what is named takes the front and the rest keep their sequence behind it. Nothing here deletes a book, a shelf or a tag; that asks the owner first, in the Library. The key buys the owner's ordinary session and no exemption: the same ownership checks, feature flag and limits decide every write, and a call that would reach another library fails here before it is sent. Every call leaves one line in logs/library-mcp.jsonl, the key and the session never in it. probe.mjs runs all five against a live library and takes back every write it makes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @manager's task in 3m 11s —— View job Review: Library MCP tools (PR #221)
Scope noteThis PR is entirely outside Findings (posted inline)
What looked solid
Nothing here blocks merging on its own; #1 is the one worth fixing before this is wired to a real agent, since it can leak note content into a log file that's presumably meant to be safe to share/inspect. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // The response is the only proof the write landed. A 200 that carries the | ||
| // old value is a failed save, not a saved one. | ||
| for (const [field, value] of Object.entries(data)) { | ||
| if (after[field] !== undefined && after[field] !== value) { | ||
| throw new LibraryError( | ||
| `The server kept ${field} as ${JSON.stringify(after[field])}`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Two related issues in this post-write check:
- False failures on server-side normalization.
after[field] !== valueis a strict comparison.noteis the field most likely to be transformed by the CMS (HTML sanitization/normalization of the<strong>/<em>/<s>/link markup the tool description promises, trailing-whitespace trimming, entity encoding). If the CMS normalizes the note even slightly, a successful write will still fail this check and be reported back to the agent as a failed save. - Content leak into the plaintext journal.
JSON.stringify(after[field])embeds the full field value in the thrownLibraryErrormessage — fornotethat can be up toMAX_NOTE_LENGTH(5000) characters. That message becomesdetailin therecord()call at the bottom ofcall().journal.mjs's truncation (LONGEST = 120, thebrief/briefArgshelpers) only applies toline.args, not todetail— so a spurious "kept different value" failure on a long note writes the entire note intologs/library-mcp.jsonlin the clear. That directly undercuts the stated design goal in journal.mjs ("A trail, not a second copy of the library... without copying the library into a log") and the CLAUDE.md claim that arguments are "cut to 120 characters."
Consider comparing only the non-text fields (rating, difficulty) strictly, treating note verification more loosely (or not at all — the reread-based verification the probe does is a better model), and truncating detail the same way args is truncated before it's journaled.
| const changed = []; | ||
|
|
||
| for (const book of add) { | ||
| if (book.tags.some(carried => carried.id === tag.id)) continue; | ||
|
|
||
| if (book.tags.length >= MAX_TAGS_PER_OBJECT) { | ||
| throw new LibraryError( | ||
| `"${book.title}" already carries ${MAX_TAGS_PER_OBJECT} tags`, | ||
| ); | ||
| } | ||
|
|
||
| await writeObject(book.id, { | ||
| tags: [...book.tags.map(carried => carried.id), tag.id], | ||
| }); | ||
| changed.push({ book: book.title, tagged: true }); | ||
| } | ||
|
|
||
| for (const book of remove) { | ||
| if (!book.tags.some(carried => carried.id === tag.id)) continue; | ||
|
|
||
| await writeObject(book.id, { | ||
| tags: book.tags | ||
| .filter(carried => carried.id !== tag.id) | ||
| .map(carried => carried.id), | ||
| }); | ||
| changed.push({ book: book.title, tagged: false }); | ||
| } |
There was a problem hiding this comment.
This loop writes sequentially and isn't transactional: if a later book in add throws (e.g. it already carries MAX_TAGS_PER_OBJECT tags), everything written before that point has already landed on the CMS, but the function throws before returning changed. call()'s catch block only reports error.message — the caller gets no indication that some books were already tagged. A retry of the same call is safe (already-tagged books are skipped), but the agent has no way to know partial success happened without a separate library_books read. Worth returning the partial changed list alongside the error, e.g. via a custom error subclass or by catching per-book and continuing rather than letting one bad reference abort the batch.
| const MAX_TAG_NAME_LENGTH = 20; | ||
| const MAX_TAG_DESCRIPTION_LENGTH = 180; | ||
| const MAX_TAGS_PER_OBJECT = 10; | ||
| const MAX_NOTE_LENGTH = 5000; |
There was a problem hiding this comment.
Minor maintainability note: MAX_TAG_NAME_LENGTH/MAX_TAG_DESCRIPTION_LENGTH/MAX_TAGS_PER_OBJECT/MAX_NOTE_LENGTH are hardcoded copies of src/constants/library/common.ts and addObjectSchema.ts (currently in sync: 20/180/10/5000). That's inconsistent with palette() a few lines down, which deliberately reads src/constants/library/tags.ts from disk at runtime specifically "so the two cannot drift." If one of these limits changes in src/constants/library/common.ts without a matching edit here, the MCP tool silently goes stale in one of two ways: rejecting input the CMS would now accept, or accepting input the CMS will now reject (paying exactly the round trip this local validation exists to avoid, per the comment on line 26). Since mcp/ intentionally has no dependency on the Next build, reading the source file the way palette() does (or extracting these four constants to a tiny JSON/constants file both sides import) would keep this from drifting silently.
| if (!args.name) throw new LibraryError('A new tag needs a name'); | ||
|
|
||
| const taken = new Set(tags.map(tag => tag.color?.toUpperCase())); | ||
| const color = | ||
| args.color || | ||
| palette().find(hex => !taken.has(hex.toUpperCase())) || | ||
| null; | ||
|
|
||
| if (!color) { | ||
| throw new LibraryError('Name a colour for this tag, as a hex value'); | ||
| } | ||
|
|
||
| const created = await createTag({ | ||
| name: args.name, | ||
| color, | ||
| description: args.description, | ||
| library: (await readLibrary()).libraryId, | ||
| }); | ||
|
|
||
| return { | ||
| created: { | ||
| id: created?.data?.id, | ||
| name: created?.data?.attributes?.name, | ||
| color: created?.data?.attributes?.color, | ||
| slug: created?.data?.attributes?.slug, | ||
| }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Nit: the create-tag path validates name length and description length locally (lines 243-256) but doesn't check the 13-tag-per-library cap that the tool's own description field advertises ("A library keeps at most 13 tags") and that CLAUDE.md documents. tags (the full list) is already in scope here — a tags.length >= 13 check before calling createTag would match the pattern already used for the other limits and save the round trip the comment on line 26 says local validation exists to avoid.
A production write through the MCP is the owner's word relayed. Each write tool now takes an optional instruction, the words the change was asked in, and every journal line names the client that asked, from the handshake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @manager's task in 1m 14s —— View job Review: Library MCP tools (PR #221) — re-check after new commit
What changed since the last reviewOne new commit,
This is small, additive, and does what it says. No new correctness or security issues:
Prior findings — still open (unchanged by this commit)This commit didn't touch the handler bodies, so the four items from the original review still apply at their new line numbers:
None of these are new, and none block merging on their own — the CMS remains the source of truth and will reject anything that slips past local validation. #1 is the one worth fixing before this is wired to a real agent, since it's the one that can leak note content into a log file meant to be safe to inspect/share. |
The limits are read from the constants the forms read, so they cannot drift; a new tag is refused locally at the thirteen-tag cap; tagging many books answers for each by name and fails as a whole only when nothing changed; the post-write check folds whitespace before comparing a note and puts no value in its message, and the journal cuts details as it cuts arguments. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review: Library MCP tools (PR #221) — re-check after commit
|
|
Review dispositions, commit 04210d3:
The probe ran again against staging library 6 after the change: all eight steps pass, every write taken back. |
# Conflicts: # CLAUDE.md
|
Claude finished @manager's task in 1m 9s —— View job Review: Library MCP tools (PR #221) — re-check after commit
|
1 participant
Five tools an agent holds a library by: list books with their notes, create or edit a tag, put a tag on a book or take it off, set the order the tag's books stand in, write a book's note and rating. Nothing here deletes anything.
Nothing is deployed by this PR. The tools need
keepsimple-cms-new#feat/library-agent-sessionon the target CMS before a key means anything.Proof
All five run against a live library, staging library 6 (LibraryReview), and every write is taken back:
Re-read afterwards: book 26 back at rating 5, difficulty easy, its five original tags and its 1261-character note. The protocol itself was exercised over stdin: initialize, tools/list, tools/call.
NOT TESTED: the key exchange end to end, because the CMS side is not deployed. Its own logic is covered by a probe in that PR.
What it costs
No dependency, no change under
src/, nothing in the Next build.mcp/is plain Node, run by an agent's MCP client.🤖 Generated with Claude Code