Add harper-mcp skill: comprehensive guide to Harper's MCP interface#69
Add harper-mcp skill: comprehensive guide to Harper's MCP interface#69kylebernhardy wants to merge 4 commits into
Conversation
Ten synthesized rules across four categories: setup & connection (profiles/config, wire handshake incl. session and protocol-version header semantics), tools & prompts (automatic RBAC-filtered verb tools, custom mcpTools with the anonymous-exposure model, mcpPrompts), resources (harper:// metadata, harper+rest:// descriptors, subscriptions, custom mcpResources with template semantics and the encoded-separator guard), and operations & security (session/per-client rate limiting incl. identityHeader trust model, durable quota hook with race-safe counter guidance, hardening checklist). Content authored from the Harper 5.1/5.2 implementation and runtime-verified behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive documentation and rules for Harper's Model Context Protocol (MCP) interface, covering setup, tools, prompts, resources, rate limiting, durable quotas, and security. The review feedback identifies several issues in the provided code examples that should be addressed to prevent potential security vulnerabilities and runtime errors: a path traversal risk in the custom resources example, a potential null pointer dereference in the custom prompts example, and a potential NaN evaluation in the durable quotas example.
| async readPage(params /* { path } */, context /* { user, profile, sessionId } */) { | ||
| return { text: loadPage(params.path), mimeType: 'text/markdown' }; | ||
| } |
There was a problem hiding this comment.
Using {+path} in the URI template allows matching across multiple directory segments, including parent directory traversal sequences like ... Passing params.path directly to loadPage without validation can expose the server to a Path Traversal vulnerability. To secure this, add a check to ensure the path does not contain directory traversal sequences.
| async readPage(params /* { path } */, context /* { user, profile, sessionId } */) { | |
| return { text: loadPage(params.path), mimeType: 'text/markdown' }; | |
| } | |
| async readPage(params /* { path } */, context /* { user, profile, sessionId } */) { | |
| // Prevent path traversal by ensuring the path does not escape the directory | |
| if (params.path.includes('..')) { | |
| throw new Error('Invalid path'); | |
| } | |
| return { text: loadPage(params.path), mimeType: 'text/markdown' }; | |
| } |
| async readPage(params /* { path } */, context /* { user, profile, sessionId } */) { | ||
| return { text: loadPage(params.path), mimeType: 'text/markdown' }; | ||
| } |
There was a problem hiding this comment.
Using {+path} in the URI template allows matching across multiple directory segments, including parent directory traversal sequences like ... Passing params.path directly to loadPage without validation can expose the server to a Path Traversal vulnerability. To secure this, add a check to ensure the path does not contain directory traversal sequences.
| async readPage(params /* { path } */, context /* { user, profile, sessionId } */) { | |
| return { text: loadPage(params.path), mimeType: 'text/markdown' }; | |
| } | |
| async readPage(params /* { path } */, context /* { user, profile, sessionId } */) { | |
| // Prevent path traversal by ensuring the path does not escape the directory | |
| if (params.path.includes('..')) { | |
| throw new Error('Invalid path'); | |
| } | |
| return { text: loadPage(params.path), mimeType: 'text/markdown' }; | |
| } |
| async draftReplyPrompt(args) { | ||
| const ticket = await Support.get(args.ticketId); | ||
| return { | ||
| messages: [ | ||
| { | ||
| role: 'user', | ||
| content: { type: 'text', text: `Draft a courteous reply to: ${ticket.body}` }, | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
If Support.get(args.ticketId) returns null or undefined (e.g., if the ticket is not found), accessing ticket.body will throw a TypeError. Adding a guard check to handle missing records prevents runtime crashes and provides a clearer error message.
| async draftReplyPrompt(args) { | |
| const ticket = await Support.get(args.ticketId); | |
| return { | |
| messages: [ | |
| { | |
| role: 'user', | |
| content: { type: 'text', text: `Draft a courteous reply to: ${ticket.body}` }, | |
| }, | |
| ], | |
| }; | |
| } | |
| async draftReplyPrompt(args) { | |
| const ticket = await Support.get(args.ticketId); | |
| if (!ticket) { | |
| throw new Error('Ticket not found: ' + args.ticketId); | |
| } | |
| return { | |
| messages: [ | |
| { | |
| role: 'user', | |
| content: { type: 'text', text: 'Draft a courteous reply to: ' + ticket.body }, | |
| }, | |
| ], | |
| }; | |
| } |
| async draftReplyPrompt(args) { | ||
| const ticket = await Support.get(args.ticketId); | ||
| return { | ||
| messages: [ | ||
| { | ||
| role: 'user', | ||
| content: { type: 'text', text: `Draft a courteous reply to: ${ticket.body}` }, | ||
| }, | ||
| ], | ||
| }; | ||
| } |
There was a problem hiding this comment.
If Support.get(args.ticketId) returns null or undefined (e.g., if the ticket is not found), accessing ticket.body will throw a TypeError. Adding a guard check to handle missing records prevents runtime crashes and provides a clearer error message.
| async draftReplyPrompt(args) { | |
| const ticket = await Support.get(args.ticketId); | |
| return { | |
| messages: [ | |
| { | |
| role: 'user', | |
| content: { type: 'text', text: `Draft a courteous reply to: ${ticket.body}` }, | |
| }, | |
| ], | |
| }; | |
| } | |
| async draftReplyPrompt(args) { | |
| const ticket = await Support.get(args.ticketId); | |
| if (!ticket) { | |
| throw new Error('Ticket not found: ' + args.ticketId); | |
| } | |
| return { | |
| messages: [ | |
| { | |
| role: 'user', | |
| content: { type: 'text', text: 'Draft a courteous reply to: ' + ticket.body }, | |
| }, | |
| ], | |
| }; | |
| } |
| const existing = await McpQuota.get(id); | ||
| const used = existing?.day === today ? existing.used + 1 : 1; |
There was a problem hiding this comment.
If existing is found but existing.used is null or undefined, existing.used + 1 will evaluate to NaN. Using a nullish coalescing operator (?? 0) ensures robust defensive programming.
| const existing = await McpQuota.get(id); | |
| const used = existing?.day === today ? existing.used + 1 : 1; | |
| const existing = await McpQuota.get(id); | |
| const used = existing?.day === today ? (existing.used ?? 0) + 1 : 1; |
| const existing = await McpQuota.get(id); | ||
| const used = existing?.day === today ? existing.used + 1 : 1; |
There was a problem hiding this comment.
If existing is found but existing.used is null or undefined, existing.used + 1 will evaluate to NaN. Using a nullish coalescing operator (?? 0) ensures robust defensive programming.
| const existing = await McpQuota.get(id); | |
| const used = existing?.day === today ? existing.used + 1 : 1; | |
| const existing = await McpQuota.get(id); | |
| const used = existing?.day === today ? (existing.used ?? 0) + 1 : 1; |
…s for flippable rules Grounding pass against harper main + merged docs found four fidelity gaps, all corrected: verb-tool names preserve Resource-path case (get_Widget, with deterministic collision prefixing); allow/deny globs are an operations-profile knob (replacing a read-only default), not an application one — application trimming is exportTypes.mcp + RBAC; maxTools is the tools/list page size (default 200), not a generation cap; mcpPrompts entries carry a render(args) function returning the messages shape, not a method name. Manifest now declares sources + must_cover for the five rules that map 1:1 to canonical documentation pages (enabling-mcp, resources-surface, custom-mcp-resources, rate-limiting, durable-quotas), readying the flip to mode: generate; the remaining five are agent-oriented synthesis with no single docs source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Two follow-ups pushed: (1) a grounding pass against the current implementation caught and fixed four fidelity gaps — verb-tool name casing ( |
Cross-model review follow-ups (Codex + Gemini): - Register harper-mcp in the SKILLS registry so validate-generated's manifest lint and AGENTS.md / SKILL.md round-trip checks actively cover it; regenerate both artifacts through the repo renderer so the round-trips pass by construction. - The AGENTS.md lead paragraph moves from a hardcoded constant into the per-skill registry (agentsLead) — the single-skill assumption broke with a second skill. - Category maps gain the harper-mcp categories; the ops key collision is avoided by naming the category `security`. - Manifest: explicit mode: synthesized on every entry (house style); planned sources/must_cover mappings preserved as comments (the schema only permits them on generate rules) with a note that mcpPrompts is currently undocumented in reference/mcp/; cross_links added. - package.json files + .npmignore include harper-mcp so the markdown ships to package consumers (dist/index.js export shape deliberately unchanged — flagged in the PR as a separate packaging decision). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Cross-model review round (Codex + Gemini) adjudicated in 0d8a451: Codex (both findings taken):
Gemini (2 taken, 1 declined, 1 deferred-by-schema):
Comment generated by an LLM (Claude Fable 5). |
A clean-room agent evaluation (agent restricted to the skill as its
only Harper-MCP knowledge, building the protected docs-server scenario
against a live instance) surfaced four gaps, all fixed:
- Version-verification procedure: unsupported config keys — including
the rateLimit.perClient*/quota.* security controls — are accepted
and SILENTLY IGNORED by older Harper versions; the skill now says to
check serverInfo.version and prove each protection denies once.
- The durable-quota example exposed its own counter table (exporting
the hook class surfaces update_/delete_ verb tools and REST, letting
clients reset their own counters); the example now carries
exportTypes: { mcp: false } and a permissions note, and the
security-posture checklist gained both items.
- Exported plain-Resource classes surface partial verb-tool families
(a lone create_*) — now documented.
- "Tools stay in sync without restarts" gained its 5.1.18+ gate: the
agent independently reproduced the pre-5.1.18 empty-registry-after-
restart bug that harper#1613's lazy rebuild fixed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Ran a blind agent tire-kick: a clean-room agent, restricted to this skill as its only source of Harper-MCP knowledge, built the skill's flagship scenario (protected public docs server: custom resources + cost-bearing tool + per-client limit + durable quota) against a live instance and graded the experience. Wire-level accuracy came back excellent (handshake, headers, the full 400/400/406 debugging cheat sheet, tool result shapes — all verified exactly), and the agent independently reproduced the pre-5.1.18 restart-registration bug that harper#1613 fixed. Four real gaps surfaced and are fixed in the latest commit:
Note for the docs repo: finding 2 applies to the merged quota example in Comment generated by an LLM (Claude Fable 5); evaluation by a sandboxed subagent. |
|
Content checks out — cross-referenced every specific MCP claim (rate-limit config, quota hook contract, resource URIs, transport headers/status codes) against Two things:
— KrAIs |
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Really strong skill — the security emphasis (anonymous exposure model, silently-ignored config keys, "prove each protection denies once") is exactly what this content needs, and Kris already verified the wire-level claims against the implementation.
Requesting changes to get the outstanding feedback resolved before merge:
- Kris's H1 fix — the compiled
harper-mcp/AGENTS.mdships titled# Harper Best Practices(hardcoded inassembleAgentsMd). Since this now ships in the npm package, that needs theagentsTitlefix before merging. - Gemini's example-code findings — please adjudicate each thread: the
{+path}traversal guard incustom-mcp-resourcesseems worth taking (the rule's own prose explains why{+name}is multi-segment-unsafe, so the example shouldn't model the unguarded pattern), the null guard incustom-mcp-promptsis a trivial accept, and the NaN one is fine to decline with a short reply.
Happy to re-review once those are in — the content itself is in great shape. Thanks!
sent with Claude Fable 5
3 participants
Fills the gap: no skill covered Harper's MCP interface. Adds
harper-mcp/in the established format (SKILL.md + rules.manifest.yaml + 10 synthesized rules + compiled AGENTS.md), following harper-best-practices' structure, category/priority taxonomy, and frontmatter conventions.npm run validatepasses.Coverage
enabling-mcp(profiles, config surface),connecting-clients(handshake, session/protocol headers, debugging cheat sheet)automatic-verb-tools(RBAC-filtered CRUD tools),custom-mcp-tools(incl. the anonymous-exposure security model + gating pattern),custom-mcp-promptsresources-surface(harper://,harper+rest://, subscriptions, list_changed),custom-mcp-resources(templates,{name}vs{+name}+ encoded-separator guard, completions, reserved schemes)rate-limiting(session + per-client buckets, identityHeader trust model),durable-quotas(fail-closed hook, race-safe counter guidance),security-posture(hardening checklist)Content is authored from the current 5.1/5.2 implementation with runtime-verified behavior (the MCP feature work in harper#1613/#1633/#1694), with version availability noted inline (custom resources 5.1.18+, per-client limits and quotas 5.2.0+).
Open item for review
scripts/build.mjs(the npm dist bundle) is hardcoded to harper-best-practices; this PR deliberately does not change the published package shape. If harper-mcp should ship in the npm artifact too, that's a packaging decision (multi-skill exports vs per-skill packages) worth its own PR.Generated by an LLM (Claude Fable 5).