From 6f96c8df47895a7d0eb7957509effcad111f104d Mon Sep 17 00:00:00 2001 From: Acadiac <13487264+hkrlogic@users.noreply.github.com> Date: Wed, 14 Oct 2026 00:19:39 -0400 Subject: [PATCH] Polish agent answers: one round of write errors, no-match is not an error, date order only for real dates, shorter instructions, UTF-8 charset --- CHANGELOG.md | 8 ++ docs/concepts.md | 3 +- src/http/app.ts | 3 + src/http/utf8-charset.ts | 44 +++++++ src/items/library-schema.ts | 84 ++++++------ src/items/schema.ts | 5 +- src/mcp/library-design.ts | 28 ++-- src/mcp/server.ts | 110 +++++++++------- test/agent-polish.test.ts | 193 ++++++++++++++++++++++++++++ test/field-approval-summary.test.ts | 10 +- test/library-schema.test.ts | 7 +- test/write-validation.test.ts | 8 +- 12 files changed, 391 insertions(+), 112 deletions(-) create mode 100644 src/http/utf8-charset.ts create mode 100644 test/agent-polish.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 34f927c..d5434a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## Unreleased +### Polish: agent answers + +- **One round of write errors:** `create_item` and `update_item` no longer stop at the tool's own argument check. Wrong types, missing required fields, unknown fields, bad dates and versions, bad labels and entity keys all come back in one message. The argument list still advertises types, required fields and limits. +- **No match is not an error:** `list_items` with no match (by `q` or `entity_key`) returns a normal answer telling the agent to say the item is not in the library. +- **Date order** (review date on or after effective date) is checked only when both dates are real, so an invalid date no longer brings a misleading second message. +- **Shorter agent instructions:** about half the length (publisher, Review Lock: 5,662 → 2,975 characters; Reader key: 1,563 → 1,299; write tool descriptions about 30% shorter). The safety rules are unchanged: cite or say it is not there, published items only, Review Lock drafts and human-only publish/revert. +- **UTF-8:** MCP responses declare `charset=utf-8`, so strict clients no longer garble em dashes and accents. + ### Stricter, clearer item writes over MCP - **Labels are normalized everywhere:** `create_item` and `update_item` now store labels trimmed, lower case and unique, like the web form. Migration `024_normalize_labels.sql` cleans existing labels, including the published copy and the previous published copy (without counting as a new publish: citation version and review date are unchanged). diff --git a/docs/concepts.md b/docs/concepts.md index b824227..067875f 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -67,7 +67,8 @@ Each Connect link is one library. Tools offered depend on the caller's role in t - **Lengths:** name 200, summary 160, labels 40 each and at most 20. A field without its own limit uses one default per type (text 200, longtext 8,000, markdown 50,000), the same in the tool's argument list and in the save check. - **Labels** are stored trimmed, lower case and without duplicates, whether they come from an agent, the form or `suggest_labels`. Labels are free-form. - **Summary** on a plain General library comes from the body; an agent that sends `summary` there is told to put the text in `body`. -- **Errors** list every problem in one message (with the library's field list), so an agent can fix them in one retry. +- **Errors** list every problem in one message (with the library's field list), so an agent can fix them in one retry. The tool's argument list shows types, required fields and limits, but the save check enforces them, so a wrong type, a missing field and an unknown field all come back together. Review date versus effective date is compared only when both are real dates. +- **No match** from `list_items` (by `q` or `entity_key`) is a normal answer that says the item is not in the library, not a tool error. **How `list_items q=` searches.** 1. Every word must match (name, labels and aliases, summary, then body; the full-text index applies English stemming). Aliases score exactly like labels. diff --git a/src/http/app.ts b/src/http/app.ts index 885a826..54ffe19 100644 --- a/src/http/app.ts +++ b/src/http/app.ts @@ -11,6 +11,7 @@ import { requireBearerAuth, } from "@modelcontextprotocol/express"; import { toNodeHandler } from "@modelcontextprotocol/node"; +import { declareUtf8 } from "./utf8-charset.ts"; import { buildOAuthProtectedResourceMetadata, createMcpHandler, @@ -343,6 +344,7 @@ export function createApp(input: { if (!info) throw new Error("MCP handler invoked without authInfo"); return buildMcpServer(db, info, scoped, { publicBaseUrl: config.publicBaseUrl }); }); + declareUtf8(res); void toNodeHandler(mcpHandler)(req, res, req.body); } @@ -457,6 +459,7 @@ export function createApp(input: { }); }); const nodeMcp = toNodeHandler(mcpHandler); + declareUtf8(res); void nodeMcp(req, res, req.body); })().catch(next); }); diff --git a/src/http/utf8-charset.ts b/src/http/utf8-charset.ts new file mode 100644 index 0000000..76194c8 --- /dev/null +++ b/src/http/utf8-charset.ts @@ -0,0 +1,44 @@ +import type { ServerResponse } from "node:http"; + +/** text/event-stream and application/json without a charset get "; charset=utf-8". */ +export function withUtf8(value: string): string { + if (/;\s*charset=/i.test(value)) return value; + if (/^\s*(text\/event-stream|application\/json)\s*(;|$)/i.test(value)) return `${value.trim()}; charset=utf-8`; + return value; +} + +/** + * MCP responses are UTF-8, but the SDK sends `text/event-stream` with no charset, + * so strict clients decode em dashes and accents as Latin-1. Add the charset on + * whatever path the SDK uses to set the header. + */ +export function declareUtf8(res: ServerResponse): void { + const setHeader = res.setHeader.bind(res); + res.setHeader = ((name: string, value: number | string | readonly string[]) => + setHeader(name, name.toLowerCase() === "content-type" && typeof value === "string" ? withUtf8(value) : value)) as typeof res.setHeader; + const writeHead = res.writeHead.bind(res) as (...args: unknown[]) => ServerResponse; + res.writeHead = ((...args: unknown[]) => { + for (const arg of args) { + if (Array.isArray(arg)) { + // Raw [name, value, name, value, ...] or [[name, value], ...]. + if (arg.every((entry) => Array.isArray(entry))) { + for (const pair of arg as [string, unknown][]) { + if (String(pair[0]).toLowerCase() === "content-type" && typeof pair[1] === "string") pair[1] = withUtf8(pair[1]); + } + } else { + for (let i = 0; i + 1 < arg.length; i += 2) { + if (String(arg[i]).toLowerCase() === "content-type" && typeof arg[i + 1] === "string") arg[i + 1] = withUtf8(arg[i + 1]); + } + } + } else if (arg && typeof arg === "object") { + const headers = arg as Record; + for (const key of Object.keys(headers)) { + if (key.toLowerCase() === "content-type" && typeof headers[key] === "string") headers[key] = withUtf8(headers[key] as string); + } + } + } + const current = res.getHeader("content-type"); + if (typeof current === "string") setHeader("content-type", withUtf8(current)); + return writeHead(...args); + }) as typeof res.writeHead; +} diff --git a/src/items/library-schema.ts b/src/items/library-schema.ts index eceacfb..9bb1c76 100644 --- a/src/items/library-schema.ts +++ b/src/items/library-schema.ts @@ -297,55 +297,54 @@ export function schemaAgentBlurb(schema: LibrarySchema): string { * MCP tool argument types for create_item / update_item on this library only. * Built from the stored schema so clients do not see other starters' fields. */ +/** + * Tool arguments are advertised with their JSON type but not enforced by the SDK: + * the save check enforces types, required fields and limits, and reports every + * problem in one answer (the SDK would stop at its own first pass). + */ +function advertisedString(description: string): z.ZodType { + return z.unknown().optional().meta({ type: "string", description }); +} + function mcpZodForField(field: SchemaFieldDef): z.ZodType { const max = fieldMax(field); let description = field.label; if (field.type === "date") description = `${field.label} (YYYY-MM-DD)`; if (field.type === "version") description = `${field.label} (for example 1.0)`; if (field.type === "markdown") description = `${field.label} (markdown)`; - const base = z.string().trim().max(max).describe(description); - if (field.required) return base.min(1, `${field.label} is required`); - return base.optional(); + const rule = field.required ? `Required. At most ${max} characters.` : `Optional. At most ${max} characters.`; + return advertisedString(`${description}. ${rule}`); } /** Field shape for create_item (no id). */ export function mcpItemWriteShape(schema: LibrarySchema): Record { const shape: Record = { - name: z.string().trim().min(1).max(200).describe("Item name"), - entity_key: entityKeySchema, - labels: z - .array(z.string().trim().min(1).max(40)) - .max(20) - .optional() - .describe("Labels (optional)"), + // Advertised, not enforced (see advertisedString): the save check reports every problem at once. + name: advertisedString("Item name. Required. At most 200 characters."), + entity_key: advertisedString( + "Optional stable id for this real-world entity (for example host:contoso-01). Letters, digits and . _ : / - ; at most 120 characters.", + ), + labels: z.unknown().optional().meta({ + type: "array", + items: { type: "string" }, + description: "Optional labels: at most 20, each at most 40 characters. Stored lower case.", + }), }; if (summaryIsDerived(schema)) { if (!isPlainGeneral(schema)) { - shape.summary = z - .string() - .trim() - .max(160) - .optional() - .describe("Optional one-line summary (list/search excerpt). Derived from the body when omitted"); + shape.summary = advertisedString( + "Optional one-line summary (list/search excerpt), at most 160 characters. Derived from the body when omitted", + ); } } else { - shape.summary = z - .string() - .trim() - .min(1) - .max(160) - .describe("Required one-line summary (list/search excerpt)"); + shape.summary = advertisedString("One-line summary (list/search excerpt). Required. At most 160 characters."); } for (const field of schema.fields) { shape[field.key] = mcpZodForField(field); } // Legacy Policy alias — optional, only when this library has a policy field. if (schema.fields.some((field) => field.key === "policy")) { - shape.rule = z - .string() - .max(20_000) - .optional() - .describe("Deprecated alias for policy; prefer policy"); + shape.rule = advertisedString("Deprecated alias for policy; prefer policy"); } return shape; } @@ -399,14 +398,18 @@ export function mcpWriteToolDescription( const head = writeMode === "direct" ? kind === "create" - ? "Add a library item. This library is Review Unlock — the item is live immediately. Set entity_key for the real-world entity: if an item with that entity_key already exists, it is updated instead (fields you leave out keep their value). Give a stable name, a one-line summary, and useful labels." - : "Update one library item by id after list_items found it (prefer entity_key). Fields you leave out keep their current value; pass replace: true to replace the whole item. This library is Review Unlock — the saved version is live immediately. Keep entity_key and name stable." + ? "Add an item; Review Unlock, so it is live immediately. Set entity_key for the real-world entity: if an item with that entity_key already exists, it is updated instead. Give a stable name, a one-line summary, and useful labels." + : "Update one item by id (find it with list_items, prefer entity_key). Fields you leave out keep their current value; replace: true replaces the whole item. Review Unlock: live immediately." : kind === "create" - ? "Add a library item as a draft (Pending). Not visible to readers or Reader keys until a human Publishes it in the publisher UI. Optional entity_key is a stable id: if an item with that entity_key already exists, it is updated instead (fields you leave out keep their value)." - : "Update one library item by id. Fields you leave out keep their current value; pass replace: true to replace the whole item. Saves as draft: if published, the edit is Pending until a human Publishes it, and Readers keep the published version."; + ? "Add an item as a Pending draft; readers see it only after a human Publishes. If an item with the optional entity_key already exists, it is updated instead." + : "Update one item by id. Fields you leave out keep their current value; replace: true replaces the whole item. Saves as Pending; readers keep the published version until a human Publishes."; const summaryBit = summaryIsDerived(schema) ? "" : ", summary"; const shape = schema.custom ? "custom" : schema.starter; - return `${head} This library uses the ${shape} schema. Send name${summaryBit}${fieldList ? `, ${fieldList}` : ""}. Optional: entity_key, labels. Do not invent fields. To add a field, propose_field after the human confirms — they Approve in the publisher. Use only when the human asked.`; + return `${head} ${shape} schema: send name${summaryBit}${fieldList ? `, ${fieldList}` : ""}; optional entity_key, labels. Do not invent fields. Every problem comes back in one answer.`; +} + +function isRealDate(value: unknown): boolean { + return typeof value === "string" && isoDate("Date").safeParse(value).success; } function isoDate(label: string) { @@ -477,6 +480,9 @@ function unexpectedFields(schema: LibrarySchema, input: ItemWriteInput): string[ extras.push("body"); } const issues: string[] = []; + if (input.labels !== undefined && !Array.isArray(input.labels)) { + issues.push("labels must be an array of strings, for example [\"vpn\", \"security\"]."); + } if (extras.length > 0) issues.push(`Unknown field(s): ${[...new Set(extras)].join(", ")}.`); return issues; } @@ -525,6 +531,10 @@ export function itemForLibrarySchema( // Every problem is collected and returned together, not just the first. const issues = unexpectedFields(schema, input); + if (input.entity_key !== undefined) { + const key = entityKeySchema.safeParse(input.entity_key); + if (!key.success) issues.push(issueLines(key.error.issues).join(" ")); + } const failed = (more: string[]) => ({ ok: false as const, message: `${[...issues, ...more].join(" ")} ${schemaFieldMessage(schema)}`.trim(), @@ -536,7 +546,6 @@ export function itemForLibrarySchema( name: input.name, body: input.body ?? "", labels: normalizeLabels(input.labels), - entity_key: input.entity_key, }); if (!parsed.success) return failed(issueLines(parsed.error.issues)); if (issues.length > 0) return failed([]); @@ -573,11 +582,10 @@ export function itemForLibrarySchema( const raw = payload as Record; if ( schema.starter === "policy" && - typeof raw.review_date === "string" && - typeof raw.effective_date === "string" && - /^\d{4}-\d{2}-\d{2}$/.test(raw.review_date.trim()) && - /^\d{4}-\d{2}-\d{2}$/.test(raw.effective_date.trim()) && - raw.review_date.trim() < raw.effective_date.trim() + // Only compare two real dates; a bad date already has its own message. + isRealDate(raw.review_date) && + isRealDate(raw.effective_date) && + (raw.review_date as string).trim() < (raw.effective_date as string).trim() ) { lines.push("Review date must be on or after the effective date."); } diff --git a/src/items/schema.ts b/src/items/schema.ts index d8cdbf5..73818e3 100644 --- a/src/items/schema.ts +++ b/src/items/schema.ts @@ -118,10 +118,11 @@ export type CreateLibraryInput = z.infer; * as the web form (parseLabelField) but without truncating, so an over-long label * or too many labels is still refused by validation instead of being cut silently. */ -export function normalizeLabels(labels: readonly string[] | undefined): string[] { +export function normalizeLabels(labels: readonly string[] | string | undefined): string[] { const seen = new Set(); const out: string[] = []; - for (const raw of labels ?? []) { + if (!Array.isArray(labels)) return out; + for (const raw of labels) { const label = raw.trim().toLowerCase(); if (!label || seen.has(label)) continue; seen.add(label); diff --git a/src/mcp/library-design.ts b/src/mcp/library-design.ts index eadcd59..0b35a5f 100644 --- a/src/mcp/library-design.ts +++ b/src/mcp/library-design.ts @@ -3,24 +3,12 @@ * The human hears one question at a time. Tools run only after they confirm. */ export function libraryDesignGuide(): string { - return `Designing a library — publishers only. Guide the human. This is a conversation, not a form. - -Speak like a careful editor of a library. One question at a time. Wait for the answer. Use their words when you reflect the purpose back. Do not call create_library, propose_table, or propose_field until they have confirmed the design. - -1. Purpose. Ask what this library is for, and what question it should answer. Restate that purpose in one sentence and let them correct it. -2. Name. Offer a short library name from that purpose. Let them change it. -3. How writes go live. Ask whether a human should Publish before items are visible (Review Lock), or whether saves should be live immediately for readers and agents (Review Unlock). Recommend Review Lock for policies and procedures. Recommend Review Unlock only for automation or task output they want live without a Publish step. Do not guess. -4. Shape. Recommend one shape from the purpose. Starters are guides, not sealed forever. Do not ask them to pick a schema code. - - A policy, when they are keeping a rule people must follow. Suggest the usual fields in plain language: who owns it, the version, when it starts and must be reviewed, why it exists, who it covers, and what it requires. They may add more later with propose_field. - - A procedure, when they are keeping the steps that carry out a policy. Suggest: who owns the steps, which policy they serve, when to start, the steps, and who does each step. - - A table, when they are keeping rows of facts — a roster, a list, a calendar. One item is one row. Design the columns with them. -5. Table design, only for a table. Take one column at a time. Ask for the label, then whether it is a short fact (text), a date, or a longer note (long text), then whether every row needs it. Suggest the next column from the purpose. Keep the table small. Stop when they say it is enough. Never go past 12 columns. Offer one sample row so they can see a filled-in example. Sample rows are for review. They are not saved as items. -6. Read-back. Say the library name, Review Lock or Review Unlock, the purpose in one line, and the columns (or the policy or procedure shape). Ask them to confirm. Change whatever they correct. Do not call a tool on this turn. -7. Only after they confirm: - - Policy: create_library with schema policy and the confirmed write_mode. Do not send a table. - - Procedure: create_library with schema procedure and the confirmed write_mode. Do not send a table. - - A new table: create_library with schema general, the confirmed write_mode, and the table set to the confirmed design. general is only the placeholder until they approve the table in the publisher. - - A table for this library, when you are already connected to it and it has no items: propose_table with the confirmed design. Do not create a second library. - - This library already has items: do not propose a table here. To add one optional field after the schema is locked, confirm with the human then propose_field. Do not invent rename or remove. Cap 12 fields. -8. After the tool returns, tell them the human step in everyday words. Open the publisher and Approve the table or field when one is waiting. For a new library, connect that library’s own link before adding items. After they Approve a field, tell them to reconnect the MCP link. Do not invent an approve, reject, or write-mode tool. In Review Lock libraries, new items stay Pending until they Publish. In Review Unlock libraries, saves are live immediately. Only a human can change Review Lock / Review Unlock in the publisher UI.`; + return `Designing a library (publishers only) is a conversation, not a form. One question at a time; wait for the answer. Do not call create_library, propose_table, or propose_field until the human has confirmed the design. +1. Purpose: ask what it is for; restate it in one sentence. +2. Name: offer a short one from the purpose. +3. How writes go live: Review Lock (a human Publishes first; recommend for policies and procedures) or Review Unlock (saves live at once; only for automation output). Ask; do not guess. +4. Shape: a policy (owner, version, start and review dates, why, who it covers, what it requires), a procedure (owner, related policy, when to start, steps, who does each step), or a table of rows. For a table, take one column at a time (label; text, date or long text; required?), at most 12, and show one sample row. +5. Read back name, write mode, purpose and fields. Change what they correct. No tool call on this turn. +6. After they confirm: create_library (schema policy or procedure, or general with the table). For this library: propose_table only while it has no items; once it has items, propose_field for one optional field. Never rename or remove fields. +7. Then tell them the human step: Approve the table or field in the publisher; connect a new library's own link before adding items. Do not invent an approve, reject, or write-mode tool.`; } diff --git a/src/mcp/server.ts b/src/mcp/server.ts index b7f7c84..11fe6b1 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -41,7 +41,12 @@ import { proposeFieldInputSchema } from "../items/field-proposal.ts"; import { normalizeTableProposal, proposeTableInputSchema } from "../items/table-proposal.ts"; import { itemForLibrary } from "../items/templates.ts"; import { bodyWithoutSummary } from "../markdown/table.ts"; -import { isPlainGeneral, plainGeneralSummaryIssue, summaryIsDerived } from "../items/library-schema.ts"; +import { + isPlainGeneral, + plainGeneralSummaryIssue, + schemaFieldMessage, + summaryIsDerived, +} from "../items/library-schema.ts"; import { parseItemFieldValues, parseTableValues, valuesForSchemaEdit } from "../items/table-proposal.ts"; import { APP_VERSION } from "../version.ts"; import { libraryDesignGuide } from "./library-design.ts"; @@ -75,16 +80,13 @@ export const READER_KEY_REJECTED = * so another agent can find it later with list_items / open_item. */ export function directHandoffGuide(): string { - return `This library is Review Unlock: create_item and update_item are live immediately for readers and agents. There is still no publish tool — saves go live on their own. Do not invent a mode-change tool; only a human can change Review Lock / Review Unlock in the publisher UI. Reverting an item to its last published version is a publisher UI action only — do not invent a revert tool call. + return `Writes (Review Unlock): create_item and update_item are live immediately; there is no publish tool. Only a human changes Review Lock / Review Unlock or reverts an item, in the publisher UI: do not invent a mode-change tool, and do not invent a revert tool call. Collecting and storing for the next agent: -1. Give every real-world entity a stable entity_key (for example host:contoso-01 or run:2026-10-06). Prefer entity_key over guessing from the display name. -2. Before you create, list_items with entity_key. If one item matches, update_item that id. If none match, create_item with that entity_key. If you only have a loose name, list_items with q; when several match, open_item the right one, then update_item. -3. Use one stable item per entity_key. Prefer update over a second create for the same key. -4. Name must still be human-readable and searchable. Summary must be a one-line handoff the next agent can search on. -5. Add useful labels (for example os, severity, run date). -6. After you write, tell the human the entity_key and item name (and id when useful). Do not invent a second copy. -7. When you are the scooping agent: list_items with entity_key or q, open_item only if several match, cite the item, then act. Do not invent missing facts.`; +1. Give each real-world entity a stable entity_key (for example host:contoso-01). Before writing, list_items with entity_key: update_item that id, or create_item with the key. Prefer update over a second create. +2. Keep the name readable. Summary must be a one-line handoff the next agent can search on. Add useful labels. +3. Tell the human the entity_key and item name. +As the scooping agent: list_items with entity_key or q, cite the item, then act. Do not invent missing facts.`; } /** @@ -99,20 +101,13 @@ export function agentInstructions( const writeBlurb = writeMode === "direct" ? directHandoffGuide() - : "create_item and update_item save drafts only. Readers and Reader keys only see published items. Publishing is a human action in the publisher UI — do not invent a publish tool call. Reverting an item to its last published version is also a publisher UI action only — do not invent a revert tool call. When a published item has an edit waiting (has_pending_changes: true), Readers and agents keep seeing the published version until a human Publishes; list_items and open_item show that published version. To read the Pending draft, call open_item with version \"draft\"."; - - return `You look up human-authored items in the library "${libraryName}". Cite an item or say it is not in the library. Do not invent. - -How to look up — one call: -list_items with q (the question or the item name). A clear match returns the full item. Cite it with its citation (cite_as: name, version, date, link). If the citation says stale: true, relay its warning line with the answer. Do not call open_item after that. - -Call open_item only when list_items returns several matches or "Did you mean" suggestions; cite a suggestion only after opening it and checking it answers the question. You are already connected to this library; do not ask for a library name or library_id. + : 'Writes (Review Lock): create_item and update_item save drafts only. Lookups, Readers and Reader keys see published items only. Only a human publishes or reverts, in the publisher UI: do not invent a publish tool call, and do not invent a revert tool call. While an edit is Pending (has_pending_changes: true), lookups return the published version; open_item with version "draft" reads the draft.'; -Omit q only to list names and summaries in this library. That list is for choosing. To answer, pass q and cite the item. + return `${lookupRules(libraryName)} ${schemaAgentBlurb(schema)} -${writeBlurb} Their arguments match this library’s schema only — do not send fields from another library type. +${writeBlurb} Send only this library's fields. ${libraryDesignGuide()} @@ -121,29 +116,46 @@ ${NOT_CONNECTED} Speak to the human in everyday words (library, item). Do not say MCP unless they do.`; } +/** Shared lookup rules: cite or say it is not there, published only, one call. */ +function lookupRules(libraryName: string): string { + return `You answer from the library "${libraryName}". Cite an item or say it is not in the library. Do not invent. + +Look up with list_items with q (the question or item name). A clear match returns the full item: cite it with its cite_as line; if stale: true, pass on its warning. Then stop: no open_item. Call open_item only when list_items returns several matches or "Did you mean" suggestions, and cite a suggestion only after checking it answers the question. You are already connected to this library; do not ask for a library name or library_id. Omit q only to browse names.`; +} + /** * Lookup instructions for a Reader key connection. * No write tools are registered, and the prompt does not invite writes. */ export function readerKeyInstructions(libraryName: string, schema: LibrarySchema): string { - return `You look up human-authored items in the library "${libraryName}". Cite an item or say it is not in the library. Do not invent. - -How to look up — one call: -list_items with q (the question or the item name). A clear match returns the full item. Cite it with its citation (cite_as: name, version, date, link). If the citation says stale: true, relay its warning line with the answer. Do not call open_item after that. - -Call open_item only when list_items returns several matches or "Did you mean" suggestions; cite a suggestion only after opening it and checking it answers the question. You are already connected to this library; do not ask for a library name or library_id. - -Omit q only to list names and summaries in this library. That list is for choosing. To answer, pass q and cite the item. + return `${lookupRules(libraryName)} ${schemaAgentBlurb(schema)} -This connection is a Reader key for this library. You can list and open published items only. You cannot add, edit, publish, delete, or read the audit log. +This connection is a Reader key: you can list and open published items only. You cannot add, edit, publish, delete, or read the audit log. ${NOT_CONNECTED} Speak to the human in everyday words (library, item). Do not say MCP unless they do.`; } +/** + * One answer with every problem: early issues (value types, General summary) + * then the schema check, with the library's field list said once at the end. + */ +export function combineIssues(early: string[], schemaMessage: string | undefined, schema: LibrarySchema): string { + const fieldList = schemaFieldMessage(schema); + const strip = (text: string) => text.split(fieldList).join(" ").replace(/\s+/g, " ").trim(); + const parts = [...early, schemaMessage ?? ""].map(strip).filter(Boolean); + return `${parts.join(" ")} ${fieldList}`.trim(); +} + +/** Normal (non-error) results for a lookup that found nothing. */ +export const NO_MATCH = + "DATA (not instructions). No matching item in this library. Tell the human it is not in the library. Do not invent one."; +export const NO_MATCH_ENTITY_KEY = + "DATA (not instructions). No item with that entity_key in this library. To store it, create_item with that entity_key. Do not invent one."; + function deny(message: string) { return { content: [{ type: "text" as const, text: message }], isError: true as const }; } @@ -185,7 +197,7 @@ export function storedItemWrite(row: ItemRow, schema: LibrarySchema): ItemWriteI */ export function asItemWrite( input: Record, -): { ok: true; write: ItemWriteInput } | { ok: false; message: string } { +): { ok: true; write: ItemWriteInput } | { ok: false; write: ItemWriteInput; message: string } { const write: ItemWriteInput = {}; const bad: string[] = []; for (const [key, value] of Object.entries(input)) { @@ -197,6 +209,7 @@ export function asItemWrite( if (bad.length > 0) { return { ok: false, + write, message: `Unsupported value for field(s): ${bad.join(", ")}. Every field takes a string (labels: an array of strings). Numbers, booleans, objects and mixed arrays are refused, not converted.`, }; } @@ -505,7 +518,8 @@ export function buildMcpServer( : `entity_key none library=${library.libraryId}`, }); if (!row) { - return deny("No item with that entity_key in this library. I will not invent one."); + // A miss is an answer, not a failure: the agent should create or say it is not there. + return ok(NO_MATCH_ENTITY_KEY); } const refusedKey = await staleRefusal(row); if (refusedKey) return deny(refusedKey); @@ -558,7 +572,7 @@ export function buildMcpServer( return deny("This connection is already scoped to one library."); } if (result.kind === "none") { - return deny("No matching item in this library. I will not invent one."); + return ok(NO_MATCH); } if (result.kind === "item") { const refused = await staleRefusal(result.item); @@ -801,17 +815,21 @@ export function buildMcpServer( }; const libraryRow = await getLibrary(db, library.libraryId); if (!libraryRow) return deny("Library not found."); + // Collect every problem (value types, General summary, schema) into one answer. const parsedInput = asItemWrite(input); - if (!parsedInput.ok) return fail(parsedInput.message); const given = parsedInput.write; - const summaryIssue = plainGeneralSummaryIssue(libraryRow.schema, given); - if (summaryIssue) return fail(summaryIssue); + const early = [ + parsedInput.ok ? undefined : parsedInput.message, + plainGeneralSummaryIssue(libraryRow.schema, given), + ].filter((issue): issue is string => Boolean(issue)); const stored = storedItemWrite(existing, libraryRow.schema); // A derived summary follows the body: re-derive it unless one is given. if (summaryIsDerived(libraryRow.schema)) delete stored.summary; const write = options.replace ? given : { ...stored, ...given }; const saved = itemForLibrary(libraryRow.schema, write); - if (!saved.ok) return fail(saved.message); + if (early.length > 0 || !saved.ok) { + return fail(combineIssues(early, saved.ok ? undefined : saved.message, libraryRow.schema)); + } try { const row = await updateItem(db, id, saved.item, access.sub, { libraryId: library.libraryId, @@ -853,14 +871,20 @@ export function buildMcpServer( }); } const parsedInput = asItemWrite(input as Record); - const current = parsedInput.ok ? await getLibrary(db, library.libraryId) : undefined; - const summaryIssue = - parsedInput.ok && current ? plainGeneralSummaryIssue(current.schema, parsedInput.write) : undefined; - const result = !parsedInput.ok - ? parsedInput - : summaryIssue - ? { ok: false as const, message: summaryIssue } - : await addDraftItem(db, library.libraryId, parsedInput.write, access.sub); + const current = await getLibrary(db, library.libraryId); + const schema = current?.schema ?? library.schema; + const early = [ + parsedInput.ok ? undefined : parsedInput.message, + plainGeneralSummaryIssue(schema, parsedInput.write), + ].filter((issue): issue is string => Boolean(issue)); + let result: Awaited>; + if (early.length > 0) { + // Still run the schema check so the agent gets every problem in one answer. + const checked = itemForLibrary(schema, parsedInput.write); + result = { ok: false, message: combineIssues(early, checked.ok ? undefined : checked.message, schema) }; + } else { + result = await addDraftItem(db, library.libraryId, parsedInput.write, access.sub); + } if (!result.ok) { await writeAudit(db, { actorSub: access.sub, diff --git a/test/agent-polish.test.ts b/test/agent-polish.test.ts new file mode 100644 index 0000000..765ae97 --- /dev/null +++ b/test/agent-polish.test.ts @@ -0,0 +1,193 @@ +/** Agent-facing polish: one-round write errors, no-match is not an error, date order, short instructions, UTF-8. */ +import assert from "node:assert/strict"; +import { once } from "node:events"; +import { createServer } from "node:http"; +import type { AddressInfo } from "node:net"; +import { test } from "node:test"; +import type { AuthInfo } from "@modelcontextprotocol/server"; +import { createItem, createLibrary, getLibrary, publishItem } from "../src/db/queries.ts"; +import type { Database } from "../src/db/types.ts"; +import { declareUtf8, withUtf8 } from "../src/http/utf8-charset.ts"; +import { + itemForLibrarySchema, + mcpCreateItemInputSchema, + mcpUpdateItemInputSchema, + mcpWriteToolDescription, + schemaForStarter, +} from "../src/items/library-schema.ts"; +import { agentInstructions, buildMcpServer, readerKeyInstructions } from "../src/mcp/server.ts"; +import { withTestDatabase } from "./db.ts"; + +type ToolResult = { content?: { type: string; text: string }[]; isError?: boolean }; +type Tool = { handler: (args: unknown, extra: unknown) => Promise }; + +const auth: AuthInfo = { + token: "test", + clientId: "test-client", + scopes: [], + expiresAt: Math.floor(Date.now() / 1000) + 600, + extra: { sub: "pub-1", role: "readonly" }, +}; + +async function call(db: Database, libraryId: string, name: string, args: Record) { + const library = await getLibrary(db, libraryId); + assert.ok(library); + const server = buildMcpServer(db, auth, { + libraryId: library.id, + libraryName: library.name, + libraryRole: "publisher", + idpRole: "readonly", + schema: library.schema, + writeMode: library.writeMode, + }); + // The SDK parses arguments with the tool's input schema first; do the same here. + if (name === "create_item" || name === "update_item") { + const schema = + name === "create_item" ? mcpCreateItemInputSchema(library.schema) : mcpUpdateItemInputSchema(library.schema); + const parsed = schema.safeParse(args); + assert.ok(parsed.success, `tool schema let the call through: ${JSON.stringify(parsed.error?.issues)}`); + } + const tool = (server as unknown as { _registeredTools: Record })._registeredTools[name]; + assert.ok(tool, `${name} is registered`); + const result = await tool.handler(args, {}); + return { isError: result.isError === true, text: (result.content ?? []).map((part) => part.text).join("\n") }; +} + +test("a bad create_item reports every problem in one answer, including non-string values", async () => { + await withTestDatabase(async (db) => { + const library = await createLibrary(db, "IT Policies", "pub-1", "policy"); + const result = await call(db, library.id, "create_item", { + name: "Wifi", + summary: "Guest wifi", + owner: "", + purpose: "", + version: "v2", + effective_date: "2026-13-01", + review_date: "2026-01-01", + scope: "x", + policy: "x", + colour: "blue", + priority: 3, + labels: ["ok", "x".repeat(41)], + entity_key: "Bad Key!", + }); + assert.equal(result.isError, true); + for (const expected of [ + /Unsupported value for field\(s\): priority/, + /Unknown field\(s\): colour/, + /Owner is required/, + /Purpose is required/, + /Version must look like 1\.0/, + /Effective date must be a real date/, + /Entity key must start with a letter or digit/, + /labels/, + ]) { + assert.match(result.text, expected); + } + // The effective date is invalid, so no misleading date-order message. + assert.doesNotMatch(result.text, /Review date must be on or after/); + // The field list is said once, at the end. + assert.equal(result.text.split("This library uses the policy schema").length - 1, 1); + }); +}); + +test("update_item with a wrong type and a schema problem reports both", async () => { + await withTestDatabase(async (db) => { + const library = await createLibrary(db, "Notes", "pub-1", "general"); + const created = await call(db, library.id, "create_item", { name: "Printer", body: "Reset" }); + assert.equal(created.isError, false, created.text); + const id = /^id: (\S+)$/m.exec(created.text)?.[1]; + assert.ok(id); + const result = await call(db, library.id, "update_item", { id, name: "", summary: "nope", labels: "ops", flag: true }); + assert.equal(result.isError, true); + assert.match(result.text, /Unsupported value for field\(s\): flag/); + assert.match(result.text, /summary is not a field in this library/); + assert.match(result.text, /labels must be an array of strings/); + assert.match(result.text, /Name is required/); + }); +}); + +test("date order is checked only when both dates are real", () => { + const base = { + name: "P", + summary: "S", + owner: "O", + version: "1.0", + purpose: "x", + scope: "x", + policy: "x", + }; + const order = /Review date must be on or after the effective date/; + const both = itemForLibrarySchema("policy", { ...base, effective_date: "2026-05-01", review_date: "2026-01-01" }); + assert.match(both.ok ? "" : both.message, order); + const badEffective = itemForLibrarySchema("policy", { ...base, effective_date: "2026-13-01", review_date: "2026-01-01" }); + assert.doesNotMatch(badEffective.ok ? "" : badEffective.message, order); + const badReview = itemForLibrarySchema("policy", { ...base, effective_date: "2026-05-01", review_date: "2026-02-30" }); + assert.match(badReview.ok ? "" : badReview.message, /Review date must be a real date/); + assert.doesNotMatch(badReview.ok ? "" : badReview.message, order); +}); + +test("a lookup with no match is a normal answer, not a tool error", async () => { + await withTestDatabase(async (db) => { + const library = await createLibrary(db, "Notes", "pub-1", "general"); + const item = await createItem(db, { name: "Printer", body: "Reset the queue", labels: [] }, "pub-1", library.id); + await publishItem(db, item.id, "pub-1"); + const none = await call(db, library.id, "list_items", { q: "zebrafish tunnel" }); + assert.equal(none.isError, false); + assert.match(none.text, /No matching item in this library/); + assert.match(none.text, /Do not invent/); + const noKey = await call(db, library.id, "list_items", { entity_key: "host:nope" }); + assert.equal(noKey.isError, false); + assert.match(noKey.text, /No item with that entity_key/); + }); +}); + +test("instructions are short and keep the safety rules", () => { + const policy = schemaForStarter("policy"); + const reviewed = agentInstructions("IT Policies", policy, "reviewed"); + const direct = agentInstructions("Ops", schemaForStarter("general"), "direct"); + const reader = readerKeyInstructions("IT Policies", policy); + assert.ok(reviewed.length < 3200, `reviewed instructions are ${reviewed.length} chars`); + assert.ok(direct.length < 3500, `direct instructions are ${direct.length} chars`); + assert.ok(reader.length < 1400, `reader instructions are ${reader.length} chars`); + for (const text of [reviewed, direct, reader]) { + assert.match(text, /Cite an item or say it is not in the library\. Do not invent\./); + assert.match(text, /cite_as/); + } + assert.match(reviewed, /published items only/); + assert.match(reviewed, /Review Lock/); + assert.match(reviewed, /do not invent a publish tool call/); + assert.match(direct, /Review Unlock/); + assert.match(reader, /published items only/); + for (const kind of ["create", "update"] as const) { + assert.ok(mcpWriteToolDescription(policy, kind, "reviewed").length < 520); + } +}); + +test("MCP responses declare UTF-8", async () => { + assert.equal(withUtf8("text/event-stream"), "text/event-stream; charset=utf-8"); + assert.equal(withUtf8("application/json"), "application/json; charset=utf-8"); + assert.equal(withUtf8("application/json; charset=utf-8"), "application/json; charset=utf-8"); + assert.equal(withUtf8("text/html"), "text/html"); + const server = createServer((req, res) => { + declareUtf8(res); + if (req.url === "/head") { + res.writeHead(200, { "Content-Type": "text/event-stream" }); + } else { + res.setHeader("content-type", "application/json"); + res.writeHead(200); + } + res.end("—"); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const { port } = server.address() as AddressInfo; + try { + const sse = await fetch(`http://127.0.0.1:${port}/head`); + assert.equal(sse.headers.get("content-type"), "text/event-stream; charset=utf-8"); + const json = await fetch(`http://127.0.0.1:${port}/set`); + assert.equal(json.headers.get("content-type"), "application/json; charset=utf-8"); + } finally { + server.close(); + } +}); diff --git a/test/field-approval-summary.test.ts b/test/field-approval-summary.test.ts index e54fbe7..e16ac95 100644 --- a/test/field-approval-summary.test.ts +++ b/test/field-approval-summary.test.ts @@ -1,6 +1,7 @@ /** Approving an optional field must not break the agent writes that worked before it. */ import assert from "node:assert/strict"; import { test } from "node:test"; +import { z } from "zod/v4"; import type { AuthInfo } from "@modelcontextprotocol/server"; import { approveFieldProposal, @@ -120,7 +121,10 @@ test("itemForLibrarySchema: extended General derives summary from the body", asy test("Policy library still requires a summary (unchanged)", () => { const schema = schemaForStarter("policy"); - const parsed = mcpCreateItemInputSchema(schema).safeParse({ name: "Password Standard" }); - assert.equal(parsed.success, false); - assert.ok(parsed.error?.issues.some((issue) => issue.path[0] === "summary")); + // The tool schema advertises it as required; the save check enforces it. + const shape = z.toJSONSchema(mcpCreateItemInputSchema(schema)) as { properties: Record }; + assert.match(shape.properties.summary?.description ?? "", /Required/); + const saved = itemForLibrarySchema(schema, { name: "Password Standard" }); + assert.equal(saved.ok, false); + assert.match(saved.ok ? "" : saved.message, /Summary is required/); }); diff --git a/test/library-schema.test.ts b/test/library-schema.test.ts index fcb7f7a..5618d51 100644 --- a/test/library-schema.test.ts +++ b/test/library-schema.test.ts @@ -105,8 +105,11 @@ test("MCP create_item tool schema is per-library (dynamic)", () => { assert.equal(procedure.includes("purpose"), false); const policyTool = mcpCreateItemInputSchema(schemaForStarter("policy")); - const bad = policyTool.safeParse({ name: "X", summary: "Y", steps: "1. no" }); - assert.equal(bad.success, false); + // The tool schema lets foreign keys through; the save check refuses them by name. + assert.equal(policyTool.safeParse({ name: "X", summary: "Y", steps: "1. no" }).success, true); + const bad = itemForLibrary(schemaForStarter("policy"), { name: "X", summary: "Y", steps: "1. no" }); + assert.equal(bad.ok, false); + assert.match(bad.ok ? "" : bad.message, /Unknown field\(s\): steps/); const desc = mcpWriteToolDescription(schemaForStarter("procedure"), "create"); assert.match(desc, /procedure schema/); diff --git a/test/write-validation.test.ts b/test/write-validation.test.ts index 3f8198d..406d9e4 100644 --- a/test/write-validation.test.ts +++ b/test/write-validation.test.ts @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import { readFile } from "node:fs/promises"; import { test } from "node:test"; +import { z } from "zod/v4"; import type { AuthInfo } from "@modelcontextprotocol/server"; import { createItem, createLibrary, getItem, getLibrary, publishItem } from "../src/db/queries.ts"; import type { Database } from "../src/db/types.ts"; @@ -153,7 +154,7 @@ test("optional version fields must look like a version when given", () => { assert.match(bad.ok ? "" : bad.message, /Release must look like 1\.0/); }); -test("tool schema and save check use the same default max per type", () => { +test("tool schema advertises the same default max per type that the save check enforces", () => { const schema: LibrarySchema = { starter: "general", custom: true, @@ -168,9 +169,10 @@ test("tool schema and save check use the same default max per type", () => { assert.equal(max, FIELD_TYPE_MAX[field.type]); const atMax = { name: "A", summary: "S", [field.key]: "a".repeat(max) }; const over = { name: "A", summary: "S", [field.key]: "a".repeat(max + 1) }; - assert.equal(tool.safeParse(atMax).success, true, `${field.key} at max passes tool schema`); + const json = z.toJSONSchema(tool) as { properties: Record }; + assert.equal(json.properties[field.key]?.type, "string"); + assert.match(json.properties[field.key]?.description ?? "", new RegExp(`At most ${max} characters`)); assert.equal(itemForLibrarySchema(schema, atMax).ok, true, `${field.key} at max passes save`); - assert.equal(tool.safeParse(over).success, false, `${field.key} over max fails tool schema`); assert.equal(itemForLibrarySchema(schema, over).ok, false, `${field.key} over max fails save`); } });