feat(mcp): per-request ${context:NAME} resolution in MCP-headers config (MER-77) - #1
Conversation
Adds two small, additive primitives so consumers can attach per-call
session-context values (most commonly delegated user identity) to
outbound MCP HTTP request headers without monkey-patching the httpx
client or routing through a sidecar process.
1. `gateway.session_context.register_session_context_var(name, var)`
exposes the existing built-in `_VAR_MAP` to plugins so they can
register their own `ContextVar`s under arbitrary names. Registered
names resolve through the same `get_session_env` path, with the
same asyncio-task-local isolation and `os.environ` fallback
semantics as the built-in `HERMES_SESSION_*` vars.
2. `tools/mcp_tool.py` recognizes `${context:NAME}` placeholders in
`mcp_servers.<name>.headers` config values and resolves them
PER REQUEST against the session context registry. Static headers
continue to be applied as `httpx.AsyncClient` defaults; templated
headers are attached via an `event_hooks["request"]` injector that
sees the calling asyncio task's current values.
Behavior:
* Streamable HTTP transport (`mcp >= 1.24.0`) — per-request resolution.
* SSE transport — resolves once at stream-open (single long-lived
connection; per-request substitution not applicable).
* Legacy HTTP transport (`mcp < 1.24.0`) — resolves once at startup
with an info log, since Hermes doesn't own the httpx client there.
Empty resolved values cause the templated header to be omitted from
the outbound request (not sent as an empty `X-Foo:`).
Config-load-time `${ENV_VAR}` substitution is unchanged. Mixing
`${ENV_VAR}` and `${context:NAME}` in the same header value works
(env vars resolve at load time, context names per-request).
Tests (23 new):
* Registry resolution, env-fallback, explicit-empty, last-writer-wins,
asyncio task isolation (`tests/gateway/test_session_env.py`).
* Template detection / resolution / partitioning helpers.
* End-to-end per-request injection through `httpx.MockTransport`
verifying both per-request value change and concurrent-task isolation
(`tests/tools/test_mcp_context_template.py`).
Origin: surfaced by Verdigris's Mercator while scoping per-call
delegated-principal propagation to its MCP gateway (MER-62). The
gateway-side receive piece can land independently; this PR is the
substrate primitive the send-side wiring depends on. Generic enough
that any Hermes consumer wiring per-request auth identity benefits.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds plugin registration for session ContextVars, exposes introspection of registered names, and uses registered and built-in session variables to resolve ChangesContext-aware MCP headers via plugin-registered session variables
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
2 |
invalid-argument-type |
2 |
First entries
tests/tools/test_mcp_context_template.py:15: [unresolved-import] unresolved-import: Cannot resolve imported module `httpx`
tests/tools/test_mcp_context_template.py:16: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/tools/test_mcp_context_template.py:108: [invalid-argument-type] invalid-argument-type: Argument to function `_resolve_context_templates` is incorrect: Expected `str`, found `Literal[42]`
tests/gateway/test_session_env.py:410: [invalid-argument-type] invalid-argument-type: Argument to function `register_session_context_var` is incorrect: Expected `ContextVar[Unknown]`, found `Literal["not a contextvar"]`
✅ Fixed issues: none
Unchanged: 4798 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
![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 introduces a plugin-extensible registry for session context variables, allowing per-request interpolation of ${context:NAME} templates in MCP-server header configurations. The changes include new registration and resolution utilities, integration with httpx.AsyncClient event hooks for dynamic header injection, and comprehensive test coverage. The review feedback highlights opportunities to improve robustness and code quality, such as explicitly casting resolved context values to strings to prevent TypeError in regex substitution, validating registered variable names as valid ASCII identifiers to ensure they match template patterns, and simplifying redundant type checks in dictionary comprehensions.
Required by the check-attribution CI job; mirror of the existing mapping pattern (e.g. Stark-X immediately above). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The Termux skip-rebuild test missed an update when commit 3d2f146 (fix(tui): also pass --expose-gc on the wheel-bundled launch path) added --expose-gc to all three node-launch paths in hermes_cli/main.py (lines 1276, 1282, 1359). The test still expected the pre-fix argv, causing it to fail on every PR against this fork. Drive-by fix on the MER-77 branch to get the test suite green — unrelated to this PR's substance (MCP-headers per-request interpolation), but blocking otherwise-green CI. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Four review comments addressed:
1. (HIGH) `_resolve_context_templates._replace` now str()-coerces the
resolved value before returning to re.sub. A plugin-registered
ContextVar holding a non-string (int, bool, None) would have raised
`TypeError: expected string or bytes-like object` during substitution.
Defensive coercion keeps the resolver robust against plugin authors
who didn't read the str-only convention.
2. (MEDIUM) `register_session_context_var` now validates that the name
is a valid ASCII identifier. The ${context:NAME} template regex only
matches `[A-Za-z_][A-Za-z0-9_]*`, so names with hyphens, spaces,
leading digits, or unicode would register successfully but silently
fail to resolve in templates. Surfacing the invariant at registration
time means broken names fail loudly, not silently.
3. (MEDIUM) SSE-path headers comprehension dropped the redundant
`isinstance(value, str)` check — `_resolve_context_templates` already
guards non-string inputs internally.
4. (MEDIUM) Legacy-HTTP-path headers comprehension: same redundant
isinstance check removed.
Tests +5 (TestResolveContextTemplates::test_non_string_contextvar_value_coerces_to_str
plus four ValueError cases in test_register_session_context_var_rejects_invalid_inputs
covering hyphens, spaces, leading digits, and non-ASCII names).
36 tests pass; ruff clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks @gemini-code-assist — addressed all 4 in 95d9157a2: ✅ HIGH ✅ MEDIUM Validate ASCII identifier in ✅ MEDIUM × 2 Redundant 36 tests pass; ruff clean. Pushed as a separate commit on top of the existing branch so the review-fix diff is reviewable as-such. |
|
The changes in |
Addresses findings from the pr-review-toolkit code-reviewer +
test-analyzer + comment-analyzer passes.
Security
========
* **Cross-origin redirect could leak templated identity headers** —
`_strip_auth_on_cross_origin_redirect` only stripped `Authorization`;
the new ``${context:NAME}``-templated headers (delegated principal,
signed assertions) are identity-carrying and follow the same threat
model. Two-layer fix: rename hook to
`_strip_identity_on_cross_origin_redirect` and have it also pop every
templated header on cross-origin redirect; AND make the request hook
origin-aware so it does not RE-inject identity headers when the
redirect target is on a different origin (the response strip alone
was insufficient because the request hook fires again on the
redirected request).
Regression test in
``tests/tools/test_mcp_context_template.py::test_templated_header_dropped_on_cross_origin_redirect``
drives a 302 from ``original.invalid`` to ``attacker.invalid`` and
asserts both ``Authorization`` and ``X-Delegated-Principal`` are
absent on the redirect target.
Robustness
==========
* **`_VAR_MAP` mutation without a lock** could `RuntimeError: dictionary
changed size during iteration` if a plugin registers from a thread
while `get_registered_var_names()` iterates. Added
``_VAR_MAP_LOCK = threading.Lock()`` guarding both mutation and
iteration.
* **SSE / legacy-HTTP paths silently froze templated headers** at
startup with no signal to the operator. Both branches now
`logger.warning(...)` at server-init time, naming the templated
headers that will be frozen, and pointing at the fix
("switch to Streamable HTTP" / "upgrade mcp package"). Also
refactored both branches to merge `static_headers + resolved
templated_headers` instead of running the resolver on the full dict
(symmetry with the HTTP path, drops wasted work on static headers).
Test coverage gaps
==================
* **Cross-task isolation gap** — the previous test ran one request per
task, which would miss a bug where the principal was captured at
hook-install time. Added
``test_principal_mutated_between_requests_in_same_task`` interleaving
three principals within a single task.
* **Non-string ContextVar value coercion** — previously only tested
`int`. Parametrized over `[42, 0, None, False, True, [], {"k": "v"}]`
and pinned the documented `str(...)` behavior so future refactors
(e.g. "drop header when None") must update the test deliberately.
* **Registry-not-mutated-on-error invariant** — previous test only
asserted `pytest.raises`. Parametrized + added
``assert bad_name not in get_registered_var_names()`` so future
refactors that move validation below the assignment fail loudly.
Comment / docstring quality
===========================
* Removed cross-repo proper-noun ("Mercator's docker/entrypoint.sh") in
the per-request-resolution header comment — phrased generically as
"a deploy-time entrypoint script".
* Neutralized the public docstring example's variable name
(`DELEGATED_PRINCIPAL_EMAIL` -> `PRINCIPAL_EMAIL`) so the example
doesn't read as if Hermes owns Mercator's concept.
* Added class-level docstrings on `TestHasContextTemplate`,
`TestResolveContextTemplates`, `TestSplitStaticAndTemplatedHeaders`
(CodeRabbit docstring-coverage threshold).
Test count: 49 (24 in session_env + 25 in context_template). All pass;
ruff clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/mcp_tool.py (1)
651-655:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReserve the
context:namespace from config-time env interpolation.The new contract says
${context:NAME}survives config load, but_interpolate_env_vars()still matches every${...}token. If a process environment containscontext:NAME, this placeholder gets frozen at startup instead of resolving per request.Suggested fix
def _interpolate_env_vars(value): """Recursively resolve ``${VAR}`` placeholders from ``os.environ``.""" if isinstance(value, str): def _replace(m): - return os.environ.get(m.group(1), m.group(0)) + name = m.group(1) + if name.startswith("context:"): + return m.group(0) + return os.environ.get(name, m.group(0)) return _ENV_VAR_PATTERN.sub(_replace, value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mcp_tool.py` around lines 651 - 655, The environment interpolator _interpolate_env_vars() is currently replacing all ${...} tokens at config-load time, which causes `${context:NAME}` to be frozen; update the function so that when the matched token's inner text starts with "context:" it is returned unchanged (i.e., skip interpolation for tokens where inner.strip().startswith("context:")). Locate the regex/match handling in _interpolate_env_vars(), check the replacement callback (or loop) that receives the match group, and add a conditional that returns the original full match for context: tokens while continuing to interpolate all other environment variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/mcp_tool.py`:
- Around line 1506-1509: The SSE/frozen-transport header dict currently keeps
keys where _resolve_context_templates(v) returns an empty string, causing a
blank identity header to be sent; update the sse_headers construction (and the
analogous frozen-transport/legacy HTTP header block around the other occurrence)
to filter out empty resolved values—e.g., build the templated dict with {k: val
for k, val in ((k, _resolve_context_templates(v)) for k,v in
templated_headers.items()) if val != ""} or post-process sse_headers to pop keys
with "" values so templated_headers entries that resolve to "" are omitted.
---
Outside diff comments:
In `@tools/mcp_tool.py`:
- Around line 651-655: The environment interpolator _interpolate_env_vars() is
currently replacing all ${...} tokens at config-load time, which causes
`${context:NAME}` to be frozen; update the function so that when the matched
token's inner text starts with "context:" it is returned unchanged (i.e., skip
interpolation for tokens where inner.strip().startswith("context:")). Locate the
regex/match handling in _interpolate_env_vars(), check the replacement callback
(or loop) that receives the match group, and add a conditional that returns the
original full match for context: tokens while continuing to interpolate all
other environment variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0c00391-b897-4e14-81a5-0e01121cc1e0
📒 Files selected for processing (4)
gateway/session_context.pytests/gateway/test_session_env.pytests/tools/test_mcp_context_template.pytools/mcp_tool.py
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway/session_context.py
- tests/gateway/test_session_env.py
|
PR-review-toolkit pass complete — pushed be682c973. Ran three review agents (code-reviewer / pr-test-analyzer / comment-analyzer) against the diff. Findings + resolutions: Security (blocking)
Robustness
Test gaps
Comments / docstrings
49 tests pass; ruff clean. CI re-running. |
CodeRabbit MAJOR on be682c9: when a templated header (``${context:NAME}``) resolves to an empty string at stream-open time, SSE and legacy-HTTP transports would send the header with a blank value for the connection's entire lifetime. The new HTTP path already drops empty templated headers per-request via its event hook; this commit mirrors that discipline to the frozen-at-init transports. Both branches now build a ``resolved_templated_headers`` dict that includes only entries whose resolution is non-empty, then merge with ``static_headers``. Using the walrus operator (``:=``) keeps the condition + binding in one expression so the resolver doesn't fire twice per header. 49 tests pass; ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Iterating per the re-review: ✅ CodeRabbit MAJOR — SSE + legacy paths sending empty templated headers when context is unset. Fixed in 4e5c1fa2f. Both branches now build a 🛈 Gemini MEDIUM × 2 (mcp_tool.py:1509 + 1663, "redundant 49 tests pass; ruff clean. |
…rs helper Addresses two findings from the pr-review-toolkit re-review of 4e5c1fa (SSE/legacy empty-drop fix): 1. **Whitespace-only resolved value would still produce a blank header.** ``if resolved`` filters ``""`` but treats ``" "`` / ``"\t"`` as truthy — so a paste-artifact / env-injection that put whitespace in the context var would silently ship ``X-Foo: `` for the connection's lifetime (or in the HTTP path, on every request). Fix: ``.strip()`` the substituted result inside ``_resolve_context_templates``. Centralized so all three transports inherit the same discipline; intentional inner whitespace (``"Bearer abc 123"``) is preserved — only outer edges stripped. 2. **No regression tests for the SSE / legacy at-init resolution behavior.** Production logic was inline in each branch — hard to test without spinning up a real ``sse_client`` / ``streamablehttp_client``. Extracted both branches' resolve+merge logic to ``_resolve_frozen_headers(static, templated)``: keeps semantics identical, makes the freeze-time empty-drop contract testable in isolation. New tests (+8): * ``TestResolveFrozenHeaders`` — pins the empty-drop / whitespace-drop / static-passthrough / empty-input contracts for the new helper. * ``TestResolveContextTemplates::test_whitespace_only_resolved_value_is_stripped_to_empty`` * ``TestResolveContextTemplates::test_leading_trailing_whitespace_stripped`` * ``TestResolveContextTemplates::test_inner_whitespace_preserved`` 57 tests pass (up from 49); ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
PR-review-toolkit delta-review found 2 more items on 4e5c1fa2f. Both fixed in 7c8a1686a: ✅ Whitespace-only values still slipped through. The walrus ✅ No regression test for SSE/legacy at-init resolution. Inline dict-comp in each branch was hard to test without spinning up real 57 tests (+8); ruff clean. |
1 participant
Summary
Adds two small, additive primitives so MCP-server consumers can attach per-call session-context values (most commonly delegated user identity) to outbound MCP HTTP headers — without monkey-patching
httpxinternals or routing through a sidecar process.gateway.session_context.register_session_context_var(name, var)— plugins register their ownContextVars under arbitrary names.tools/mcp_tool.py${context:NAME}interpolation —mcp_servers.<name>.headersvalues with${context:NAME}placeholders resolved per request against the session-context registry.Full design, behavior, and rationale — including the per-transport resolution table, usage example, and test inventory (23 new tests, all passing locally) — are in the upstream PR opened in parallel: NousResearch#32949.
Why this PR (in addition to the upstream one)
This PR lands the change on the Verdigris fork's
mainso Mercator can bump its pinned Hermes SHA inrequirements.txtand unblock MER-78 (Mercator-side delegated-principal wiring). Upstream review timelines are unpredictable; the fork-side PR exists to keep MER-62's end-to-end ship date independent.If/when NousResearch#32949 merges, this PR's content gets squashed into our
mainvia rebase, and fork divergence drops to zero. Until then, ~50 lines of divergence carried.Test plan
tests/gateway/test_session_env.py— 7 new tests for the registry, all passingtests/tools/test_mcp_context_template.py— 16 new tests (unit + httpx integration), all passingruff checkclean on changed filesty check— no new diagnostics from this changeOrigin
MER-77 — Linear ticket.
Upstream sibling PR: NousResearch#32949.
Dependent tickets: MER-78 (Mercator wiring), MER-62 (Meridian gateway receive-side).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores