Conversation
…esearch#78174) Shared gateways currently store MCP OAuth per profile+server, so Alice's token can be reused for Bob. Add mcp.oauth.identity_mode (default shared) with a fail-closed per_user mode that scopes tokens, providers, 401 refresh, live connections, breakers, and private schema-cache entries to the bound gateway principal. Empty tenant scope canonicalizes to "~"; missing identity never falls back to a shared token; hermes mcp remove cleans by-user artifacts; CLI login without a bound principal is refused. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
Circuit-breaker and 401 stubs now accept the scoped handle_401 kwargs. Manager isolation tests seed requester-scoped token files so get_or_build_provider does not require an interactive TTY. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
cursor
Bot
changed the title
run_coroutine_threadsafe copies the loop thread's ContextVars, so per_user OAuth capture would fail closed (or inherit a stale principal) on a live gateway request. Re-bind the scheduling thread's principal inside the scheduled task, pin it on MCPServerTask.start before ensure_future, and never recapture on reconnect. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
invalidate_if_disk_changed now takes hermes_home and oauth_scope so the 401 path cannot re-resolve ambient identity. The concurrent-dedup stub must accept and forward those kwargs. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Replace the bare-name live-key fallback with one fail-closed _oauth_call_target, keep manager _key a pure tuple, and resolve identity only at public API edges. Credential paths use the exact registry key; 401 recovery no longer runs against ambient shared state on a miss. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
cursor
Bot
changed the title
Passing the already-resolved registry key into _ensure_lazy_server_connected broke first-use stubs that still take only the server name. Lookup still uses the fail-closed key; lazy connect re-resolves on demand. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
|
@codex[agent] review |
Reviewed the current PR state and latest 6 commits; I don’t see an additional change needed from my side at this point. Reviewed head: |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39f0350c5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and name not in _lazy_server_tool_names | ||
| ): | ||
| try: | ||
| entry = get_cached_entry(name, config_fingerprint(cfg)) |
There was a problem hiding this comment.
On a restarted per_user gateway where the only manifest is the normal private cache written by _register_server_tools, this unbound startup lookup always uses the bare server-name key, while the cache was stored under the requester's registry token. The server is then placed in _lazy_server_configs, and later discovery under that bound requester skips it at if k in _lazy_server_configs; because no handler was registered, no first tool call can trigger the live connection. Consequently a user with valid scoped tokens/cache loses all MCP tools after restart unless an old unscoped or explicitly public cache happens to exist. The startup-to-bound-session path needs to load the requester's scoped cache or connect eagerly.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| _server_connect_errors.pop(live_key, None) | ||
| _server_connect_errors.pop(name, None) | ||
| _servers[name] = server | ||
| _servers[live_key] = server |
There was a problem hiding this comment.
Once this registry stores simultaneous Alice and Bob connections for the same logical server, both tasks record the same global tool names. If Alice's connection exhausts its reconnect budget, its unchanged _deregister_tools() removes those names from the process-wide registry and provenance map even while Bob's connection remains healthy; Bob also keeps a nonempty _registered_tool_names, so his reconnect path will not republish them. New sessions—and refreshed existing sessions—therefore lose a healthy user's MCP tools because another user's connection failed. Tool deregistration needs per-principal ownership/refcounting or must preserve names while any same-name connection is live.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| for srv_name, srv_cfg in servers.items(): | ||
| if server_uses_oauth(srv_cfg): | ||
| _oauth_protected_servers.add(srv_name) |
There was a problem hiding this comment.
When a server is changed from auth: oauth to header/no authentication and /reload-mcp runs in the same process, this set retains the server name because neither reload nor shutdown_mcp_servers() clears it. _mcp_server_uses_oauth() subsequently treats the new non-OAuth config as OAuth due to the stale membership; in per_user mode, reload discovery without a bound principal defers the server entirely, and bound calls incorrectly create requester-scoped connections. Rebuild or remove entries from this classification on config reload so the documented process-level behavior for static-header servers is restored.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
Unbound per_user startup now loads any matching requester-scoped schema cache so tool names survive a gateway restart. Bound re-register no longer skips a lazy template whose tools were never published. _deregister_tools keeps names still served by a sibling connection or the cache-backed template. OAuth classification is discarded on auth change and cleared on shutdown so header/none reloads do not stay requester-scoped. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
|
@codex[agent] review |
Ran another review pass on the latest 7 commits; I don’t see additional changes needed from my side right now. Reviewed head: |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8eaa3ed96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _server_connect_errors.pop(live_key, None) | ||
| _server_connect_errors.pop(name, None) | ||
| _servers[name] = server | ||
| _servers[live_key] = server |
There was a problem hiding this comment.
When Alice and Bob both have live per-user OAuth connections, this process-wide registry now contains both entries, but the gateway /reload-mcp path still calls shutdown_mcp_servers() over every registry value (gateway/run.py:23923). Thus either authenticated requester can disconnect the other requester's credential-bearing session, contradicting the isolation contract and disrupting concurrent calls; the reload path must avoid shutting down unrelated principal entries or be restricted to a trusted global administrator.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| """Pop lazy templates only in shared mode. Returns (fingerprint, names).""" | ||
| from tools.mcp_oauth_identity import IDENTITY_MODE_PER_USER, configured_identity_mode | ||
|
|
||
| if configured_identity_mode() == IDENTITY_MODE_PER_USER: |
There was a problem hiding this comment.
When a per-user OAuth server has published cached tools and is subsequently deleted from config.yaml, this branch retains its lazy config and tool names indefinitely. shutdown_mcp_servers() does not clear the lazy maps, and _deregister_tools() preserves names found there, so /reload-mcp leaves the deleted tool handler registered; invoking it can reconnect to the removed endpoint using the stale config. Retention is needed between principals during normal operation, but a full shutdown/reload must explicitly deregister and purge these templates.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| from tools.mcp_oauth_identity import is_registry_key_for_server | ||
|
|
||
| logical = self.name | ||
| keep = set(_lazy_server_tool_names.get(logical) or []) |
There was a problem hiding this comment.
When two principals receive different private tool manifests, this ownership protection only covers _deregister_tools(). The existing _refresh_tools() path still globally deregisters every name removed by one connection (tools/mcp_tool.py:2800-2810), so Alice receiving tools/list_changed can remove a tool that Bob's live connection still advertises. Apply the same sibling/lazy ownership check to refresh-time stale-name removal so one requester's manifest update cannot break another requester's tools.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
/reload-mcp no longer calls shutdown_mcp_servers over every live connection. In per_user mode a bound requester recycles only their OAuth sessions, process-level non-OAuth servers, and identities of servers removed from config — Alice cannot tear down Bob's OAuth session. Full shutdown and scoped reload now purge cache-backed lazy templates so a deleted server cannot stay callable. tools/list_changed refresh keeps a name registered while a sibling live connection still advertises it. Co-authored-by: Yu Ishikawa <yu-iskw@users.noreply.github.com>
3 participants
What does this PR do?
Shared Hermes gateways stored MCP OAuth tokens per profile + server (
$HERMES_HOME/mcp-tokens/<server>.json). On a multi-user gateway, Alice’s GitHub (or other OAuth) credential could be reused for Bob.This PR adds an explicit
mcp.oauth.identity_modesetting:shared(default, absent key): existing layout and behavior. Single-user CLI/TUI/desktop keep working with no config change.per_user: tokens and live connections are isolated by a bound requester principal(v1, platform, scope_id, user_id)from session ContextVars only — neveros.environ, never tool arguments. Persistence keys areu-v1-+ SHA-256 of that tuple, so raw user IDs never appear in paths.per_userfails closed when no principal is bound (CLI, TUI, desktop, and cron). Invalid values such asper-userare rejected rather than silently falling back to shared. Credential lookups use an exact registry token; they never fall back to “any connection named github.”This is a native implementation of NousResearch#78174, not a transplant of NousResearch#79449. Headless consent UX (NousResearch#78169) is out of scope.
Related Issue
Fixes NousResearch/hermes-agent#78174
Type of Change
Changes Made
tools/mcp_oauth_identity.py— typed principal/scope, fail-closed resolver, opaque persistence keys, exact registry tokensgateway/session_context.py—get_bound_session_principal()/apply_bound_session_principal(); never readsos.environfor OAuth identitytools/mcp_oauth.py—HermesTokenStoragepinshermes_home+ scope;per_userlayout undermcp-tokens/by-user/<digest>/; adminall_identitieswipe forhermes mcp removetools/mcp_oauth_manager.py— provider cache keyed by(home, server, persistence_key);_keyis a pure tuple (no ambient re-resolve)tools/mcp_tool.py— fail-closed_oauth_call_target; live maps use exact keys; MCP-loop hops re-bind the caller principal; startup does not pick a shared human credential inper_user;/reload-mcpusesreload_mcp_connections()so a boundper_userrequester cannot disconnect another principal’s OAuth sessiontools/mcp_schema_cache.py— private list/schema cache entries are principal-scoped;cacheScope=publicstays unscoped;get_startup_cached_entry()republishes tool names from any matching scoped cache without selecting credentialshermes_cli/config_defaults.py,cli-config.yaml.example,website/docs/user-guide/features/mcp.md—mcp.oauth.identity_modedocs/rfc/requester-scoped-mcp-oauth.md— locked decisionstests/tools/test_mcp_oauth_identity.py,tests/tools/test_mcp_oauth_per_user.py,tests/tools/test_mcp_loop_session_principal.pyReview follow-ups (Codex on #2)
per_userstartup loads a requester-scoped schema cache so MCP tool names survive a gateway restart (schemas only; never used as a credential selector). Bound/reload-mcpno longer skips a lazy template whose tools were never published._deregister_toolskeeps a name registered while another live connection for the same logical server still lists it, or while the cache-backed lazy template still lists it._oauth_protected_serversis add-or-discard per server in the current register batch, and is cleared onshutdown_mcp_servers(), soauth: oauth→ header/none on reload does not stay requester-scoped./reload-mcp(gateway, CLI, TUI) callsreload_mcp_connections()instead of a process-wideshutdown_mcp_servers(). Inper_userwith a bound requester, Alice’s OAuth connections, process-level non-OAuth servers, and every identity of a server removed fromconfig.yamlare recycled; Bob’s live OAuth session stays up. Shared mode and unbound CLI/TUI still take the full wipe path. Process-exit teardown still callsshutdown_mcp_servers()._deregister_toolsstarted preserving those cache names._refresh_tools(tools/list_changed) no longer globally deregisters a name another principal’s live connection still advertises. Lazy-cache names are not treated as ownership on the live-refresh path, so a truly deleted tool still drops when no sibling holds it.How to Test
mcp.oauth.identity_mode: per_user, Alice and Bob bound as different gateway requesters must get distinct token paths and must not share a live MCP connection. A call with no bound principal must fail closed rather than using Alice’s token.mcp.oauth.identity_mode(or setshared) and existing$HERMES_HOME/mcp-tokens/<server>.jsonlogin/reuse still works./reload-mcpinper_user: Alice’s reload must not close Bob’s live OAuth connection; a server removed fromconfig.yamlmust drop its lazy template and tool names.pytest;scripts/run_tests.shis CI-parity):Last run: 313 passed, 0 failed.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passThe full tree was not run as
pytest tests/ -q. The MCP OAuth / session / reload slice above was run withscripts/run_tests.sh(313 passed).Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A (docs/rfc/requester-scoped-mcp-oauth.mdinstead)Screenshots / Logs
N/A — isolation is covered by unit tests (
test_mcp_oauth_per_user.py,test_mcp_oauth_identity.py,test_mcp_loop_session_principal.py).