Skip to content

Plugin hooks for Prompts/Resources MCP paths - #73

Open
madhu-mohan-jaishankar wants to merge 4 commits into
mainfrom
feat/cpex-non-tool-hooks
Open

Plugin hooks for Prompts/Resources MCP paths#73
madhu-mohan-jaishankar wants to merge 4 commits into
mainfrom
feat/cpex-non-tool-hooks

Conversation

@madhu-mohan-jaishankar

@madhu-mohan-jaishankar madhu-mohan-jaishankar commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Pull Request

🔗 Related Issue

Closes #5258

📝 Summary

The Rust dataplane's CPEX integration only covered tools/call. A plugin such as
secrets-detection therefore protected tool traffic while prompts, resources, and
completions passed through unscanned.

This PR widens the supported hook surface from two hook names to six — adding the
prompt and resource families — and wires them into every MCP method the gateway
routes.

MCP path Hook family Pre hook Post hook
prompts/get prompt deny, replace arguments deny, rewrite message text
resources/read resource deny, rewrite URI deny, rewrite text contents
resources/subscribe resource deny, rewrite URI none by design
resources/unsubscribe resource deny, rewrite URI none by design
completion/complete prompt or resource deny deny, rewrite values
prompts/list prompt deny deny, read-only
resources/list resource deny deny, read-only
resources/templates/list resource deny deny, read-only

Ordering. Routed paths run hooks after backend routing, so plugins see the
backend-local identifier and the backend name separately and never need to know the
gateway's namespace scheme. Fan-out paths run the pre hook once before fan-out (a
denial costs zero backend traffic) and the post hook once on the merged, namespaced
result (so plugins see exactly what the client will receive, and cannot break the
routing contract by mutating pre-merge).

Scope note. resources/read and resources/list are not in the issue's path
list. See Notes below for why they are included.

Existing tool hook behavior is unchanged.


📏 Reviewability

  • This PR has one clear purpose
  • The linked issue is not labeled triage#5258 is currently labeled triage; needs triage removed before merge
  • Unrelated bugs or improvements are tracked in separate issues/PRs — two findings not yet filed, see Notes
  • Tests are included with the code they validate
  • If AI-assisted, I understand and can explain the generated changes — author to confirm

🏷️ Type of Change

  • Bug fix
  • Feature / Enhancement
  • Documentation
  • Refactor
  • Chore (deps, CI, tooling)
  • Other (describe below)

No dependencies added or changed.


🧪 Verification

This repo has no make lint / make test / make coverage targets — its Makefile
covers Docker only. The commands below are the ones CI runs (.github/workflows/ci.yml),
executed locally on rustc 1.97.1.

Check Command Status
Format cargo fmt --all --check ✅ clean
Lint cargo clippy --locked --workspace --all-targets -- -D warnings ✅ clean
Tests cargo test --locked --workspace ✅ 127 passed, 0 failed
Build cargo build --locked --workspace
Benches compile cargo bench --locked --workspace --no-run
Unused deps cargo shear --check-test-targets --deny-warnings --locked ⚠️ not run locally (not installed); no deps changed
Advisories/licenses cargo deny check advisories bans licenses ⚠️ not run locally (not installed); no deps changed
Coverage ≥ 80% ⚠️ no coverage tooling configured in this repo

Test count: 76 → 127 (+51).

Suite Tests Covers
contextforge-gateway-rs-cpex unit 23 → 29 (+6) config gate widening; prompt pre/post at the runtime layer
tests/gateway_prompt_plugins.rs 14 (new) prompts/get, prompts/list
tests/gateway_resource_plugins.rs 22 (new) resources/read, resources/list, resources/templates/list, subscribe/unsubscribe
tests/gateway_completion_plugins.rs 9 (new) completion/complete, both reference kinds

Every path is covered for: hook ordering, pre-deny (asserting the backend was never
called
), post-deny (asserting the backend did run), mutation where supported,
structural-rejection where not, and pre→post correlation-id linkage.

Three isolation tests back the "tool behavior unchanged" criterion by asserting each
hook family runs only for its own paths — e.g. a resource-only plugin config records
zero invocations across a tool call and a prompt listing.


✅ Checklist

  • Code formatted — cargo fmt --all (the template's make black isort pre-commit is Python tooling; not applicable here)
  • Tests added/updated for changes
  • Documentation updated — docs/book/src/plugins-and-policy.md rewritten; docs/book/src/mcp-method-reference.md updated per AGENTS.md's "update the matching book page in the same change"
  • No secrets or credentials committed

📓 Notes

Why resources/read and resources/list are included

resources/read returns actual resource contents: config files, database rows,
documents. Shipping resource hooks that cover template listings but not
resource reads would leave partial coverage that looks complete — an operator enabling
a redaction plugin would reasonably assume resource reads were scanned.

It is also the cleanest CMF mapping in the change: TextResourceContents { uri, mime_type, text }Resource { uri, mime_type, content }, essentially field-for-field.

resources/list is included for symmetry with the template listing at near-zero cost;
adding it let both listings share one pre/post implementation, so it removed
duplication rather than adding it.

Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>

# Conflicts:
#	crates/contextforge-gateway-rs-lib/src/gateway/mcp_service/prompts.rs
#	crates/contextforge-gateway-rs-lib/src/gateway/mcp_service/resources.rs

@lucarlig lucarlig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madhu-mohan-jaishankar, I’m requesting changes based on full-stack testing of commit 0305789 with a real MCP client → gateway → CPEX plugin → backend path.

The blocking issues are:

  1. Prompt post-hooks can remove sensitive text, but the gateway silently restores and returns the original text.
  2. The same fail-open behavior affects resources/read content.
  3. Subscription URI rewrites are recomputed during unsubscribe instead of reusing the URI selected during subscribe, so a changed or stateful rewrite can leave the backend subscription and gateway tracking entry active.

let Some(text) = message.content.iter().find_map(|part| match part {
ContentPart::Text { text } => Some(text),
_ => None,
}) else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A post-hook can remove every text part, but this continue retains the backend’s original text. In a full MCP client → gateway → CPEX plugin → backend test, the plugin cleared review of secret, yet the client still received that original value. Please treat missing text as an invalid plugin result or implement explicit redaction semantics; it must not fall back to the sensitive original.

}

for (resource, original_contents) in resources.iter().zip(original.contents.iter_mut()) {
let Some(text) = &resource.content else { continue };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A plugin can set Resource.content to None, but this branch leaves the original backend text untouched. The end-to-end test removed quarterly numbers, yet the client still received it. Please fail closed or implement explicit deletion semantics instead of restoring the original content.

// untracked, so a rewritten subscribe and unsubscribe pair resolve to the same tracking key.
let resource_uri = match &mcp_service.plugin_runtime {
Some(plugin_runtime) => {
plugin_runtime.before_unsubscribe(&resource_uri, &service_name).await?.apply_to(resource_uri)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rewritten URI selected during subscribe is not stored against the client’s original URI. An end-to-end plugin rewrote subscribe to rewritten://resource-1 and unsubscribe to rewritten://resource-2; the backend therefore never unsubscribed resource-1 and the gateway retained its tracking entry. Please persist the original-to-upstream mapping at subscribe time and consume that mapping during unsubscribe.

@gandhipratik203 gandhipratik203 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the docs work here, the payload-shape table and the mutation-limits section made this much easier to reason about. Two findings, on top of the review already open:

1. Subscribe URI rewrite misroutes notifications (resources.rs:216-225, backend_client.rs:158-171)

The subscription is tracked under the rewritten URI, and that key is re-namespaced on the way back out, so a client that subscribes to ns/X ends up receiving updates for ns/Y. The original-to-upstream mapping already suggested for the unsubscribe path would cover this as well, provided it is also consumed when forwarding notifications.

2. Empty-identifier listing marker breaks the resource_pre_fetch contract (cmf.rs:201, cmf.rs:255)

Listings signal themselves with an empty name/URI. plugins/resource_filter/resource_filter.py:88-94 in mcp-context-forge denies any schemeless URI, so it would deny every resources/list and resources/templates/list once ported. prompts/list is the weaker case of the two, since there is no part-type fallback to discriminate on. Would an explicit marker be workable instead of the empty-string sentinel?

@gandhipratik203 gandhipratik203 self-assigned this Jul 31, 2026
…mapping

Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
@madhu-mohan-jaishankar

madhu-mohan-jaishankar commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

@lucarlig @gandhipratik203 — thanks both. Fixes are in c9e1723, with a follow-up merge in f3a1f93. Three findings are fixed, one I'd like to defer with a follow-up issue, and there's a separate collision with #70 worth flagging at the end.


1 & 2 (Luca) — Fail-open when a plugin removes text (prompts/get, resources/read)

Root cause was that I destroyed the information needed to tell two cases apart. The forward mapping emits a text part for every text block, but non-text blocks map to no text part — so "no text part present" was ambiguous between "the plugin deleted it" and "this was never exposed". I resolved that ambiguity toward passthrough, which is the unsafe direction.

Both write-backs now consult the original item to disambiguate:

ContentBlock::Text(original_text) => match text {
    Some(text) => original_text.text.clone_from(text),
    // A text block is always exposed as a text part, so its absence means the
    // plugin removed it. Falling back to the backend text would return content
    // a redaction plugin stripped, so this fails closed instead.
    None => return Err(invalid_payload("Plugin removed text from a prompt message")),
},
_ if text.is_none() => {},   // non-text: no text back is expected

I went with fail closed (INVALID_PARAMS) rather than implementing deletion semantics — smaller change, cannot leak. Plugins wanting to strip content should rewrite to an empty string; that path works today and is now documented. Happy to add real deletion semantics here instead if you'd prefer.

New tests: prompt_post_hook_removing_text_fails_closed, read_resource_post_hook_removing_text_fails_closed.


3 (Luca) + 1 (Pratik) — Subscription URI rewrites

Both confirmed, same root cause, one fix.

My contract assumed a rewrite was deterministic and stateless, and — worse — my test encoded that assumption rather than testing it: the fixture rewrote to a constant, so it could never have failed. Pratik's notification finding follows from the same mistake; I tracked under the rewritten URI and then re-namespaced that outbound, so a client subscribing to ns/X would have received updates for ns/Y.

Subscriptions are now keyed by the client's URI, with the upstream URI stored alongside:

  • unsubscribe still runs its pre hook so a plugin can deny it, but addresses the backend with the URI recorded at subscribe time — never a fresh rewrite.
  • on_resource_updated maps back to the client's URI before namespacing, so updates are reported under the URI the client actually subscribed to.

New test unsubscribe_uses_the_uri_recorded_at_subscribe_not_a_fresh_rewrite uses a plugin that returns a different URI on each call — the case my original fixture couldn't express.


2 (Pratik) — Empty-identifier listing marker

Confirmed, and thanks for catching it — but we need to handle it as a follow-up rather than in this PR.

The reason is that no marker value satisfies the constraint, so it's a contract question rather than a value choice:

  • empty URI → INVALID_URI (no scheme), as you found
  • cpex://listing or any synthetic scheme → PROTOCOL_BLOCKED, since allowed_protocols defaults to ["file", "http", "https"]
  • CMF Channel is Analysis/Commentary/Final — no slot for marking a request kind

The deeper issue is that resource_pre_fetch means "a specific resource is being fetched", and a listing isn't that. Any URI-shaped value trips a plugin that reasonably assumes the payload describes a real resource.

It's also not reachable today: plugins here are statically registered Rust factories compiled in behind a cargo feature, so resource_filter.py can't run against these payloads until it's ported to Rust-native CPEX. That makes this prospective rather than a live defect — unlike the three above, which any redaction plugin would hit immediately.

Options as I see them, none of which I want to pick unilaterally:

  1. No reference part at all on listing pre-hooks. Nothing for a URI validator to reject; the listing is identified by absence, with the hook name distinguishing prompt from resource. Costs the correlation id in the pre payload (pre→post linkage still works via the CPEX context table).
  2. No pre hook on listings, post hook only. Post payloads carry real namespaced URIs that validate fine, but this loses the "denial costs zero backend traffic" property.
  3. Keep the sentinel, and accept that URI-validating plugins can't be enabled alongside listings. Weak, since our validator rejects per-plugin conditions, so there's no way to scope a plugin off listings.

I lean toward (1). Please open a follow up issue @gandhipratik203.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CF-DATAPLANE] Add CPEX hooks for non-tool MCP pass-through calls

3 participants