feat: ship MCP prompts and resources, not just tools (#371) - #392
Conversation
Review: prompts and resources (#392)Read the whole diff. This is a genuinely well-argued PR — the "prompts never fetch" rule with the FINDINGS #4 reasoning, Findings below, most-significant first. 1. Resource list entries inherit the template's
|
|
Review round applied in 83a69a8. Every finding actioned. Two were real bugs my own snapshots had encoded:
The rest: the Not taken: the The rejecting-read assertion sits in the unit tests rather than the contract suite: the eval fixture answers an unknown uuid with HTTP 200 and a not-found body where real Coolify sends a 404. Worth knowing separately. The contract gate failure was mine and is fixed: The three Now: 847 jest tests, 26 contract tests, tsc / lint / evals typecheck clean. |
Three guided workflows and two attachable reads, so a client shows workflows rather than only a list of 45 tools. Prompts (`troubleshoot_application`, `explain_failed_deploy`, `estate_health`) are pure text builders that make no API call. `prompts/get` has no elicitation channel and no error a human can act on, so a prompt that pre-fetched would turn a slash command into a stall that can fail. Worse, embedding build output in the returned message would put attacker-influenceable text in a user-role message, outside the `asUntrustedLogs` boundary the tool layer puts around exactly that text (FINDINGS #4). Naming the tool and letting the model call it keeps the log on the one path that frames it as data. A prompt may only name tools that exist in the current mode. Read-only mode drops every mutating tool, and consolidation puts two pure reads under destructive ones (`env_vars` list, `deployment` get), so this is per-mode rather than constant. `definePrompt`'s `requires` drops a whole prompt whose workflow cannot run — `explain_failed_deploy` is simply not listed on a read-only server — and `ctx.has()` drops a single step, so read-only troubleshooting keeps its log walk without naming `env_vars`. Structural, not a list to remember. Resources (`coolify://overview`, `coolify://application/{uuid}`) read through the same client as every tool call, so `deepSanitize` applies and a resource read can never be a masking bypass; the contract suite asserts byte equality with `get_application` rather than assuming it. No URI takes a `reveal`: a resource URI is a durable handle a client may cache or paste, which is the last place for an opt-in to plaintext. Fleet mode scopes every URI to its instance, because reading prod's overview believing it is staging's is the mistake a second instance invents. `get_infrastructure_overview` moved to a shared method so the tool and the resource cannot answer differently. Prompts and resources are separate lists over separate methods, so the tools/list token budget is unchanged. Contract snapshots, budgets and the masking eval extend to both surfaces in single and fleet mode. Claude-Session: https://claude.ai/code/session_01THRG6UFPUdMSARgJLcGf9a
Formatting: evals has its own prettier config, so `evals/**` must be
formatted from inside `evals/`. That is what failed the contract gate.
Review findings:
1. Listed resources inherited the template's `title`, so every
application rendered as the identical row "Application detail" —
`title` takes precedence over `name` in clients that implement it,
which defeated the only reason the listing makes API calls. Each
entry now carries its own title, and a test asserts no two listed
resources share one, since the snapshot happily encoded the
collision.
2. An empty `{instance}` capture fell through to the default instance,
because `registry.get('')` returns the default exactly as
`get(undefined)` does — a silent cross-instance read on the surface
this design argues hardest about. Now throws. Same for an empty
`{uuid}`, which would have reached `GET /applications/`, Laravel's
index route, returning every application under a URI claiming one.
3. `prompts.ts` referenced `instructions.ts`, which lives on an
unmerged sibling branch. Reworded to contrast with tool
descriptions, which are on this branch.
4. `registeredPrompts` was written and never read. It now backs a
boot-time roster check: a prompt missing from PROMPT_NAMES, or a
PROMPT_NAMES entry that never registered on a full server, throws
rather than showing up as a snapshot diff nobody reads.
5. The "names only real tools" contract check was fail-open — a prompt
naming a tool that was neither registered nor in the allowlist
passed silently. Inverted: every backticked token that is not a
registered tool is a finding, with a NOT_A_TOOL list for the
argument names the prose backticks. Forgetting to list an argument
now fails loudly; forgetting a tool no longer passes quietly.
6. Coverage: fleet application-detail reads (the one resource path
where a routing mistake serves prod under a staging URI), the
estate_health plural branch, a rejecting resource read, and an
application name containing a double quote.
7. `resources/list` fan-out cost documented and filed as #393 rather
than papered over with an unmeasured cache.
8. Prompt arguments are `JSON.stringify`d into the text, `{instance}`
gains a completion callback on both templates, and the user-role
assumption about prompt arguments is stated rather than inferred.
The rejecting-read assertion lives in the unit tests, not the contract
suite: the eval fixture answers an unknown uuid with HTTP 200 and a
not-found body where real Coolify sends a 404.
Claude-Session: https://claude.ai/code/session_01THRG6UFPUdMSARgJLcGf9a
83a69a8 to
b5e8b6a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
==========================================
+ Coverage 90.24% 90.35% +0.10%
==========================================
Files 13 14 +1
Lines 1538 1555 +17
Branches 469 476 +7
==========================================
+ Hits 1388 1405 +17
Misses 58 58
Partials 92 92 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Closes #371.
The server has exposed one of MCP's three primitives. This adds the other two: three prompts a client shows as slash commands, and two resources a client can attach. No new Coolify endpoint, no new dependency —
registerPromptandResourceTemplateare already in SDK v2.Prompts
troubleshoot_applicationquery(name, UUID or domain)diagnose_app, thenlogs, then env sanity, ending in a cause and the smallest fixexplain_failed_deploydeployment_uuiddeploymentget, which stage broke, quoting the evidenceestate_healthfind_issuesplus the overview, worst firstFleet mode adds the same optional
instancethe tools take.They make no API call
prompts/getis synchronous, has no elicitation channel and no error affordance, so a prompt that pre-fetched would turn a slash command into a multi-second stall that can fail with something the human cannot act on.The bigger reason is security. Embedding container or build output in the returned message would place attacker-influenceable text in a user-role message, outside the
asUntrustedLogsboundary the tool layer puts around exactly that text (evals/FINDINGS.md#4). Naming the tool and letting the model call it keeps the log on the one path that frames it as data. The builders are pure string functions and a test spies on the client to prove it.A prompt whose tools are missing is not listed
This was the design's one real wall. Read-only mode (#303) registers no mutating tool, and consolidation puts two pure reads under destructive tools:
env_varslist anddeploymentget. So "which tools exist" is a per-mode fact, and a prompt namingenv_varsnames a tool a read-only server does not have.Resolved structurally rather than with a comment nobody maintains.
defineToolrecords what it registered;definePrompttakesrequiresand skips the whole prompt when a required tool is absent; the builders takectx.has()and drop an individual step.Concretely, on a read-only server:
explain_failed_deployis not listed at all — the build log is reachable only throughdeployment, so the workflow cannot run, and a dead-end slash command is worse than none.troubleshoot_applicationis listed with its env step dropped.estate_healthis unchanged.Resources
coolify://overviewandcoolify://application/{uuid}, instance-scoped in fleet mode (coolify://staging/overview), because reading production's overview while believing it is staging's is the mistake a second instance invents. Every application is listed as a concrete entry so a human can pick one instead of knowing a UUID; an unreachable instance drops out of the listing rather than blanking it.Masking. Reads go through the same client as every tool call, so
deepSanitizeapplies. The contract suite asserts the resource payload equalsget_application's byte for byte rather than assuming it, with a planted fixture secret so the paired assertion can actually fail. No URI takes areveal:get_applicationhas one because a caller justifies it in the moment, whereas a resource URI is a durable handle a client may cache, re-read or paste.get_infrastructure_overviewmoved into a shared private method, so the tool and the resource cannot drift into two answers.Contract
tools/listis untouched — these are separate lists over separate methods, which is what #371 asked for. The evals harness now lists all three surfaces from one boot, with_prompts.json/_resources.jsonsnapshots and fleet variants, their own budgets, and a check that no prompt names a tool the server did not register.Verification
npm test— 844 passing, 15 suites.prompts.tsat 100% statements.npm run snapshots— 23 contract tests passing, single and fleet.npm run lint,npx tsc --noEmit, evalstypecheck+lintclean. No new warnings.sitebuild and tests pass; the new doc lands in/llms.txtand/llms-full.txt.Notes
### Addedanchor in CHANGELOG.md, so expect a conflict there on the second and later merges.https://claude.ai/code/session_01THRG6UFPUdMSARgJLcGf9a