Conversation
|
@codex[agent] review |
Review completed on |
|
To use Codex here, create a Codex account and connect to github. |
![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: 41c5fa089e
ℹ️ 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".
| @staticmethod | ||
| def _resolve_scope(oauth_scope: McpOAuthScope | None) -> McpOAuthScope: | ||
| return oauth_scope if oauth_scope is not None else resolve_oauth_scope() |
There was a problem hiding this comment.
With mcp.oauth.identity_mode: per_user, resolving an omitted scope here makes every existing interactive authorization path fail before it can obtain tokens: hermes_cli.mcp_config._reauth_oauth_server() calls manager.remove() without a bound gateway identity, and tui_gateway.mcp_oauth_sessions.start_flow() starts _worker in a new thread that neither inherits session ContextVars nor supplies oauth_scope to remove() or the subsequent probe. The dashboard/TUI flow therefore reports a missing authenticated principal, while CLI login cannot select a principal at all; propagate a validated requesting scope through these flows and use scoped storage for their snapshot/verification steps.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| with _INSTALL_LOCK: | ||
| servers = mcp_tool._servers | ||
| if not isinstance(servers, ScopedMCPServerRegistry): | ||
| servers = ScopedMCPServerRegistry(dict(servers)) | ||
| mcp_tool._servers = servers |
There was a problem hiding this comment.
When an OAuth server returns identity-dependent tool lists, these adapters isolate the connection and health maps but leave tools.registry.registry process-global. Each scoped connection still calls _register_server_tools(): a later user's discovery overwrites shared schemas for matching names, while tools available only to an earlier user remain registered and pass the server-level check_fn whenever the later user's connection is live. This exposes another user's capability metadata, sends unsupported calls to the current user's server, and mutates tool schemas underneath existing conversation snapshots; the live tool registry and stale-tool reconciliation need the same principal boundary as _servers.
AGENTS.md reference: AGENTS.md:L19-L23
Useful? React with 👍 / 👎.
| from hermes_cli.config import load_config | ||
|
|
||
| servers = (load_config() or {}).get("mcp_servers") or {} | ||
| config = servers.get(server_name) if isinstance(servers, dict) else None | ||
| return ( | ||
| isinstance(config, dict) | ||
| and str(config.get("auth") or "").strip().lower() == "oauth" |
There was a problem hiding this comment.
For an OAuth MCP supplied by a portable plugin, _load_mcp_config() merges PluginManager.get_portable_mcp_servers(), but this predicate examines only load_config()["mcp_servers"]. It consequently classifies that OAuth server as non-OAuth and reads/writes the legacy shared cache key even in per_user mode, allowing one gateway user's private tool manifest to be registered for another; determine OAuth status from the resolved server config passed through discovery or include the portable-server source in this lookup.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
fal's post-trained H3 variant — #1-ranked quality/prompt adherence/ aesthetics, 5s 768p video in under 3 seconds, $0.04/s launch pricing. - New minimax-h3-max family: minimax/h3-max/{text,image}-to-video - Inherits base-H3 wire quirks (integer duration, i2v drops aspect_ratio) but caps at 768P (480P/768P enums, no 2K/4K) and declares seed on both endpoints - New generic static_payload family flag: constant keys the endpoint requires on every request (H3 Max lists prompt_expansion_mode in its required array; sent as 'balanced') Payload asserted against the endpoint OpenAPI schema; 73/73 targeted tests green (surface matrix auto-covers the new family).
2 participants
Summary
Implements the security boundary from NousResearch#78174 against the current fork codebase rather than rebasing the outdated NousResearch#79449 implementation.
This draft intentionally focuses on NousResearch#78174 only. Headless consent delivery from NousResearch#78169 is left for a follow-up change after the credential/connection boundary is stable.
Security model
mcp.oauth.identity_modesupports backwards-compatiblesharedand opt-inper_user.per_usermode fails closed. There is no shared-token or other-user connection fallback.user_idor similar business arguments never select OAuth credentials.Isolation covered
MCPServerTasklookupValidation added
Adversarial tests cover:
Follow-up before marking ready
mcp_tool.pylifecycle/status/circuit-breaker behavior under multiple scoped connections and scope any remaining cross-user availability state where necessary.Refs NousResearch#78174.