Add harper-mcp skill: comprehensive guide to Harper's MCP interface - #69
Conversation
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.
|
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 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). |
|
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
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
|
Round 2 addressed (dec41cf + 64d2766): @kriszyp #1 / @Ethan-Arrowood #1 — H1 fix: @Ethan-Arrowood #2 — Gemini threads adjudicated per your split: the @kriszyp #2 — generate flip: agreed, and the manifest already carries the full Comment generated by an LLM (Claude Fable 5). |
Sorry, why? |
|
Fair question — "follow-up" was the wrong framing on my part. The real reason isn't process, it's that the docs sources aren't merge-ready to generate from yet:
So the sequencing that actually serves the anti-drift goal is: land the docs fixes (#576, and #575 for prompts), then flip. I'm fine doing that flip in this PR — hold it open, rebase once #576 merges, generate + verify the 6 flippable rules, keep Your call on hold-and-flip-here vs merge-now-flip-next; either way the flip is gated on #576 landing, not on merging this. Comment generated by an LLM (Claude Fable 5). |
kriszyp
left a comment
There was a problem hiding this comment.
Nice addition — this is a genuinely useful, thorough skill doc for the MCP interface, and the earlier agentsTitle fix (so harper-mcp/AGENTS.md now opens with the right H1 instead of the generic one) looks solid.
Two API-accuracy issues worth a quick fix, though neither should block merging — it sounds like this material is somewhat temporary, so flagging for a fast follow-up rather than requesting changes:
-
automatic-verb-tools.md:47(duplicated in the compiledAGENTS.md:151) shows gating MCP export viaserver.http(InternalThing, { exportTypes: { mcp: false } }). Checkingserver/http.tsonorigin/mainofdev/harper,httpServer()never readsoptions.exportTypes— this silently misregisters the resource instead of gating anything. The real mechanism isresources.set(path, Resource, { mcp: false }), which this same PR already gets right indurable-quotas.md/security-posture.md(commit9ad155d1fixed those two but missed this one). -
custom-mcp-prompts.md:28documentsrender(args)with no second parameter, but the real dispatch incomponents/mcp/transport.tscallsrender(args, { user, profile, sessionId }). Since the example serves a support-ticket body throughprompts/get(exposed to all sessions, including anonymous ones, per the rule's own prose), an author copying this as-is has no way to know they can gate oncontext.user— worth adding before someone ships the unguarded version.
Both are small, mechanical fixes (swap the registration call / add the context param plus a one-line gate example) — not blocking, just worth doing before or shortly after merge.
— Claude (Sonnet 5), reviewed via review-queue
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
I prefer hold-and-flip here. This MCP feature is already live in Harper 5.1, so the docs should be updated ASAP to be as correct as possible. Then you should absolutely be using the generated skill type to enforce that these skills are based off of that correct documentation.
sent with Claude Opus 4.8
|
Agreed on hold-and-flip — that's the right call, and it lines up with why this has been parked. The gating dependency is HarperFast/documentation#576: the So the sequence is: docs#576 merges → flip this PR to the Comment generated by kAIle (Claude Opus 4.8) |
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>
…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>
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>
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>
…d 2)
- assembleAgentsMd hardcoded '# Harper Best Practices' as the H1 —
parameterized as agentsTitle in the SKILLS registry alongside
agentsLead (kriszyp), threaded through generate/validate call sites;
harper-mcp/AGENTS.md now carries its own title.
- custom-mcp-resources example: the {+path} read now models the safe
keyed-lookup pattern with an explicit warning against unvalidated
filesystem path construction (gemini bot, seconded by Ethan — the
rule's own prose says {+name} spans segments, so the example must
not model the unguarded pattern).
- custom-mcp-prompts example: null-ticket guard (gemini bot).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The prior commit threaded it through generate-rules but the validator call site edit silently no-op'd (unasserted string replace against an already-reformatted line) — the round-trip was comparing against the default title. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…idance
A static exportTypes field on an exported class is never read (kriszyp
on documentation#576) — the wired mechanism is
server.resources.set(name, Class, { mcp: false, rest: false }).
Runtime-verified on main: hook class absent from tools/list, REST 404,
quota hook still resolves and denies. Also documents that mcp: false
excludes the class's own custom mcpTools, so cost-bearing tools belong
on a separate class.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Derive enabling-mcp, custom-mcp-tools, rate-limiting, and durable-quotas from reference/v5/mcp/* so the skill and the docs cannot drift, mirroring how harper-best-practices flipped its 1:1 rules. Gated on documentation#576, now merged. durable-quotas is rewritten from the merged "Durable quota handler" docs: the old body documented the removed `mcp.<profile>.quota.resource` config hook; the API is now the `server.setMcpQuotaHandler` registration function (harper#1821 / documentation#576). enabling-mcp's stale `quota.*` config reference is corrected to match. Also fixes a pre-existing bug @kriszyp flagged in the original review and never addressed: automatic-verb-tools showed MCP gating via `server.http(Resource, { exportTypes: { mcp: false } })`, but server.http (= httpServer, harper server/http.ts) never reads exportTypes — the mechanism is `server.resources.set(path, Resource, { mcp: false })`. Verified against harper main. Three docs-mapped rules stay synthesized with their sources/must_cover recorded as comments to uncomment on flip: - automatic-verb-tools — its source's exportTypes-gating example carries the same server.http bug; flip once the doc is corrected. - resources-surface, custom-mcp-resources — pending documentation#567, which adds the harper+rest:// scheme and the custom mcpResources section they map to. AGENTS.md and the SKILL.md index are regenerated. `npm run validate` passes, including validate-generated against the built docs. Comment generated by kAIle (Claude Opus 4.8)
9ad155d to
abced8b
Compare
The generate flip is in (
|
|
Doc fix for the Comment generated by kAIle (Claude Opus 4.8) |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
LGTM. The delta since my last review resolves both blockers — the compiled AGENTS.md H1 is now # Harper MCP, and the earlier example findings are addressed — and it lands the generate-flip we discussed (4 rules flipped to mode: generate with resolving source docs, 6 held synthesized with documented reasons). I verified the two riskiest new API claims against harper@main: server.setMcpQuotaHandler matches components/mcp/quota.ts (post #1821, including the per-profile / fail-closed / opt-in semantics), and server.resources.set(path, R, { mcp: false }) gating matches the Resources.ts signature + isMcpExposed. Both accurate.
sent with Claude Opus 4.8
kriszyp
left a comment
There was a problem hiding this comment.
It sounds like this is currently blocked; just noting that here so I don't try to re-review and have to wade through a tome to notice.
|
🎉 This PR is included in version 1.12.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
) * fix(generate): stop docs-sync from losing facts and skipping skills Three defects in the auto-sync pipeline, surfaced while reviewing #84. Fact retention. Nothing prevented a regeneration from deleting a documented constraint. validate-generated only asserted that each `must_cover` string appears somewhere in the body, which requires a human to predict every fact in advance and still passes when the term survives but the fact around it is gone. #81 responded by growing must_cover 76 -> 115 anchors; the very next run still dropped seven facts that main documents, all with anchors green. Add checkFactRetention: fail when an inline code span present in BOTH the previously committed body AND the current docs source disappears from the regenerated body. Requiring presence in the current source is what makes it safe to gate on — a fact deleted upstream is correctly dropped and never reported. Run against #84 it catches `Sec-WebSocket-Protocol: mqtt`, the `HdbError: <attribute> is not indexed...` string, and `instanceof`/`import()`/`logger` from the deleted moduleLoader behavior tables, with no false positives across all 22 generate rules. Intentional removals are recorded per rule under `allow_dropped`. Prose-only facts are still review's job; this gates the identifier, header, status-code and error-string class. Multi-skill staging. The commit step hardcoded `harper-best-practices`, so harper-mcp — added later, in #69 — was regenerated on every run and never staged. Both its rules have carried a stale sourceCommit since they were written, and an MCP-only docs change reported "No changes" and opened no PR. Stage every directory in the SKILLS registry instead, via a new skill-dirs.mjs so lib/manifest.mjs stays the single registry. Rolling-branch regeneration. The job checked out main and force-pushed over the sync branch, so every run re-sampled every drifted rule from main's stale baselines rather than skipping rules already current. That resampling is why facts appeared and disappeared between runs of the same PR, and it discarded fixes committed during review. Generate on top of the open sync branch when one exists, merging main in and failing loudly on conflict, and push with --force-with-lease. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(generate): note that fact-retention needs --docs-path A green PR-time check does not mean retention was verified: the validate-skills workflow runs plain `npm run validate` with no docs checkout, so the docs-dependent checks are skipped there. The gate bites in generate.yaml, which passes --docs-path before opening or updating the sync PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(generate): address review — git identity, PR-state gating, fences Three fixes from Kris's review on #86. Git identity before the merge. actions/checkout configures no identity, and `git merge origin/main` writes a commit whenever it is not a fast-forward — which is the normal path, since semantic-release advances main routinely. Git then exits 128 with "Committer identity unknown" and the broad handler misreported it as a merge conflict, wedging sync. Reproduced locally: exit 128, and `git ls-files --unmerged` empty, so the old message was actively wrong. Configure the App identity before the merge (the commit step still sets it; git config is idempotent), and split the handler so a real conflict names its files via --diff-filter=U while any other failure says so instead of guessing. Reuse the branch only while an open PR owns it. Remote branch existence is not the same thing: closing a rejected sync PR leaves its head branch behind, so the documented "close the PR and re-run" recovery checked out the same stale branch and replayed the conflict, or carried abandoned generated content into the next PR. Gate on `gh pr view --json state` and start clean from origin/main otherwise; --force-with-lease still refuses to clobber a push that landed after checkout. The recovery hint now matches the behaviour. Fence-aware stripping. inlineCodeSpans stripped only triple-backtick fences while its comment promised fenced blocks were excluded, so a backtick expression inside a ~~~ fence would register as a fact and deleting that example later could falsely block generation. Add stripFencedBlocks to lib/sources.mjs, where fence knowledge already lives — sliceSection has handled `{3,}`/`~{3,}` all along, so the narrower regex was also inconsistent with the module it sits next to. It follows CommonMark on delimiter character, run length, indent and info strings, and stripCode now shares it. Covered by 13 focused tests under node --test (no new dependency), wired into `npm run validate` via a `test` script. Behaviour on the current corpus is provably unchanged: old and new stripping produce identical fact sets across all 33 rule bodies, so the losses this caught on #84 still get caught. The scanner only differs on inputs the corpus does not yet contain, which is the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(generate): distinguish "no sync PR" from a failed lookup Two more from Kris's review, both confirmed. Lookup failure was indistinguishable from "no open PR". `gh pr view` exits non-zero both when no PR exists and on a transient or auth error, and `|| echo ""` collapsed the two. A GitHub hiccup while an open sync PR existed would therefore reset the branch to origin/main, and since --force-with-lease compares against the unchanged remote ref the lease matches and the push discards that PR's review fixes. Use `gh pr list --state open --head`, which exits 0 with an empty array when nothing matches, and abort when the lookup itself fails. Verified: returns 0 for an absent head and 1 for a head with an open PR, exit 0 in both cases. Time-of-check/time-of-use on a reused branch. The ownership snapshot is taken before generation, which takes minutes. If the PR is closed in that window, its content is abandoned and pushing would resurrect it, possibly into a fresh PR. The checkout step now records whether it reused the branch, and the push step re-checks ownership before overwriting, failing loudly instead. A clean run from main is unaffected. Conflicted paths were captured after the abort. `git merge --abort` clears the index, so the `git diff --diff-filter=U` substitution inside the error message ran against an already-reset index and always expanded to nothing — the message named no files exactly when someone needed them. Reproduced: unmerged=[f] before the abort, empty after. Capture into CONFLICTS first, then abort, then report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(generate): report repeat failures over REST so they stop going silent The failure reporter aborted whenever it had something to say. `gh issue comment` uses the GraphQL `addComment` mutation, which the sync App token cannot call — "GraphQL: Resource not accessible by integration (addComment)" — even though it creates issues fine over REST. Under `set -e` that killed the step, so the dedup path was strictly worse than no dedup: the first failure for a docs SHA opened an issue, and every later failure for that same SHA reported nothing at all. The 2026-09-07 scheduled run failed on the stale `Module Loading` section and left no issue, no comment, and no stated reason. Post the comment via the REST endpoint the App can actually use, and make it non-fatal: reporting must never mask the generation failure it exists to report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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).